CLI with SQL plug-in version selection #35
Conversation
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
Mebsina
requested review from
MaxKsyunz,
Swiddis,
YANG-DB,
Yury-Fridlyand,
anirudha,
dai-chen,
derek-ho,
joshuali925,
kavithacm,
mengweieric,
penghuo,
ps48 and
seankao-az
as code owners
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
…ction Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
…mically Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
|
@Mebsina CI is failing. Could you fix it first? |
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
| @@ -0,0 +1,81 @@ | |||
| { | |||
| "keywords": [ | |||
There was a problem hiding this comment.
This has to be maintained manually and binding to certain version?
There was a problem hiding this comment.
Yes, right now, it's manually added and not bind to any version. I will see if I can pull them from Lexer instead. OpenSearchPPLLexer and OpenSearchSQLLexer
There was a problem hiding this comment.
Highlevel comments on java part:
- can we remove
System.out.printlnandprintStackTracein this PR and use log4j logger instead? - for the constant values, can we make them as variables?
Example constant:
String path = isExplainQuery ? "/_explain" : "/_plugins/_ppl";
PPLQueryRequest pplRequest = new PPLQueryRequest(query, new JSONObject(), path, "");
There was a problem hiding this comment.
let's remove this module from cli codebase
| }; | ||
|
|
||
| // Check if this is an explain query | ||
| boolean isExplainQuery = query.trim().toLowerCase().startsWith("explain"); |
There was a problem hiding this comment.
why hardcode this instead of using parser? https://github.com/opensearch-project/sql/blob/e23686e58ef1d7d759342669d8b906c94b7253fa/ppl/src/main/antlr/OpenSearchPPLParser.g4#L26
There was a problem hiding this comment.
It works for PPL because it has explain statement but there is nothing for explain in the SQL Parser so I had to manually remove the explain prefix out from the query before sending it and set the path to _explain. If I send it as is then it will fail. Same thing as in curl
For example SQL:
This works
curl -X POST "http://localhost:9200/_plugins/_sql/_explain" -H "Content-Type: application/json" -d '
{
"query": "select * from sample"
}'
This fails
curl -X POST "http://localhost:9200/_plugins/_sql" -H "Content-Type: application/json" -d '
{
"query": "explain select * from sample"
}'
but for PPL it works for both _plugins/_ppl and _plugins/_ppl/_explain endpoints
curl -X POST "http://localhost:9200/_plugins/_ppl" -H "Content-Type: application/json" -d '
{
"query": "explain source=sample"
}'
| if (isPPL) { | ||
| System.out.println("Executing PPL query..."); | ||
| // For explain queries, set the path to "/_explain" | ||
| String path = isExplainQuery ? "/_explain" : "/_plugins/_ppl"; | ||
| PPLQueryRequest pplRequest = new PPLQueryRequest(query, new JSONObject(), path, ""); | ||
|
|
||
| if (isExplainQuery) { | ||
| System.out.println("Calling pplService.explain()"); | ||
| pplService.explain(pplRequest, explainListener); | ||
| } else { | ||
| System.out.println("Calling pplService.execute()"); | ||
| pplService.execute(pplRequest, queryListener, explainListener); | ||
| } | ||
| } else { | ||
| System.out.println("Executing SQL query..."); | ||
|
|
||
| if (isExplainQuery) { | ||
| // Remove "explain" prefix | ||
| String actualQuery = query.substring(7).trim(); | ||
| System.out.println("SQL explain query for: " + actualQuery); | ||
|
|
||
| String path = "/_explain"; | ||
| SQLQueryRequest sqlRequest = new SQLQueryRequest(new JSONObject(), actualQuery, path, ""); | ||
|
|
||
| System.out.println("Calling sqlService.execute() with explain path"); | ||
| sqlService.execute(sqlRequest, queryListener, explainListener); | ||
| } else { | ||
| // Regular SQL query | ||
| String path = "/_plugins/_sql"; | ||
| SQLQueryRequest sqlRequest = new SQLQueryRequest(new JSONObject(), query, path, ""); | ||
|
|
||
| System.out.println("Calling sqlService.execute()"); | ||
| sqlService.execute(sqlRequest, queryListener, explainListener); | ||
| } | ||
| } |
There was a problem hiding this comment.
let's refactor this code with design patterns, by learning Strategy Pattern and Factory Pattern.
| System.out.println("Query type: " + (isPPL ? "PPL" : "SQL")); | ||
|
|
||
| try { | ||
| CountDownLatch latch = new CountDownLatch(1); |
There was a problem hiding this comment.
why introduce CountDownLatch?
| return "Query execution fails because " + errorRef.get(); | ||
| } | ||
|
|
||
| // Handle the response based on the query type |
There was a problem hiding this comment.
let's refactor this section:
- use design patterns same as above
- result processing logic should be seperated from the query execution class
| import software.amazon.awssdk.identity.spi.AwsCredentialsIdentity; | ||
| import software.amazon.awssdk.regions.Region; | ||
|
|
||
| // AWS Request Signing Interceptor by acm19 |
There was a problem hiding this comment.
Is this class copied from somewhere?
There was a problem hiding this comment.
Yes, using this from AwsRequestSigningApacheV5Interceptor but need to modify it because RestHighLevelClient generates BasicHttpRequest and it doesn't have its body content also the interceptor sign ClassicHttpRequest instead. I had to attach the body content manually. For Http4, it works fine. I will update the documentation regarding this modification.
| * Client class for creating OpenSearch clients with different authentication methods using HTTP5 | ||
| * for OpenSearch SQL plug-in version 3 | ||
| */ | ||
| public class Client5 { |
There was a problem hiding this comment.
let's rename client4 and client5
| * Client class for creating OpenSearch clients with different authentication methods using HTTP4 | ||
| * for OpenSearch SQL plug-in version 2 | ||
| */ | ||
| public class Client4 { |
There was a problem hiding this comment.
Let's refactor both client4 and client5 with better organization, separation of concerns, and improved readability.
- consider use builder pattern to build OpenSearchClient
- offload common logic into a function
| try { | ||
| // Determine which client class to use based on useHttp5 flag | ||
| String clientClassName = useHttp5 ? "client.http5.Client5" : "client.http4.Client4"; | ||
| Class<?> clientClass = Class.forName(clientClassName); | ||
|
|
||
| if (useAwsAuth) { | ||
| // Call createAwsClient(awsEndpoint) | ||
| Method method = clientClass.getMethod("createAwsClient", String.class); | ||
| return (OpenSearchClient) method.invoke(null, awsEndpoint); | ||
| } else if (protocol != null && protocol.equalsIgnoreCase("https")) { | ||
| // Call createHttpsClient(host, port, username, password, ignoreSSL) | ||
| Method method = | ||
| clientClass.getMethod( | ||
| "createHttpsClient", | ||
| String.class, | ||
| int.class, | ||
| String.class, | ||
| String.class, | ||
| boolean.class); | ||
| return (OpenSearchClient) method.invoke(null, host, port, username, password, ignoreSSL); | ||
| } else { | ||
| // Call createHttpClient(host, port) | ||
| Method method = clientClass.getMethod("createHttpClient", String.class, int.class); | ||
| return (OpenSearchClient) method.invoke(null, host, port); | ||
| } | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("Failed to create OpenSearchClient: " + e.getMessage(), e); | ||
| } |
There was a problem hiding this comment.
this logic can be refactored with factory build pattern
| public ResponseFormatter<QueryResult> getFormatter( | ||
| String formatName, | ||
| @Named("pretty") SimpleJsonResponseFormatter prettyFormatter, | ||
| @Named("compact") SimpleJsonResponseFormatter compactFormatter, | ||
| CsvResponseFormatter csvFormatter, | ||
| JdbcResponseFormatter jdbcFormatter, | ||
| RawResponseFormatter rawFormatter) { | ||
| if (formatName == null || formatName.isEmpty()) { | ||
| // Default to JSON | ||
| return prettyFormatter; | ||
| } | ||
|
|
||
| switch (formatName.toLowerCase()) { | ||
| case "csv": | ||
| return csvFormatter; | ||
| case "json": | ||
| return prettyFormatter; | ||
| case "compact_json": | ||
| return compactFormatter; | ||
| case "jdbc": | ||
| return jdbcFormatter; | ||
| case "raw": | ||
| return rawFormatter; | ||
| case "table": | ||
| return jdbcFormatter; | ||
| default: | ||
| return prettyFormatter; | ||
| } | ||
| } |
Signed-off-by: cnoramut <[email protected]>
I removed the |
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
| console = Console() | ||
|
|
||
|
|
||
| class Config: |
There was a problem hiding this comment.
Why is there a config class in both java and python? can we consolidate them?
There was a problem hiding this comment.
Because Python config is for CLI but Java config is for SQL Plug-in settings in the GatewayModule to use. They both use the same config.yaml file, though.
| self.language_mode = "ppl" | ||
| self.is_ppl_mode = True | ||
| self.format = "table" | ||
| self.is_vertical = False | ||
| self.latest_query = None |
There was a problem hiding this comment.
let's define the constants separately instead of hardcoding them here
src/main/python/opensearchsql_cli/literals/opensearch_literals.py
Outdated
Show resolved
Hide resolved
| console = Console() | ||
|
|
||
|
|
||
| class VerifyCluster: |
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
Signed-off-by: cnoramut <[email protected]>
* cli with sql version selection * update README.md * update .gitignore for saved.txt * update README.md and default 3.1 sql version * update README.md for build.gradle instruction * get config settings from previous CLI * use opensearchpy like the previous CLI for indices and checking connection * update usage.gif * update config handling empty string * get version from sonatype and dynamically build fat jar * http5 for v3+ * add http4 for v2 and update build.gradle to pick dependency more dynamically * use datasources submodule jar locally * update README.md * add necessary packages * add --local and --remote commands for local jars dev/test * add --local and --remote commands for local jars dev/test * remove -v in version to avoid confusion * allow user to clone to their chosen directory * update artifact v3 to v4 * update remote to use --branch, default main * update dev guide * no longer need local datasources jar * update rebuild logic * will need to fix datasources in maven snapshot * not using wrapper for sigv4 interceptors * Python 3.8 to 3.9 * Python 3.8 to 3.12 * add packages * fix error handling * use logback instead of sysout * simplified python * refactoring queryexecution and formatter * use builder for client and separate functions * update gradle for default build * parse calcite plain dynamically * refactored with dictionary maps * create logs dir if doesnt exist * fix test fail --------- Signed-off-by: cnoramut <[email protected]> Co-authored-by: Mebsina <[email protected]>
4 participants
Description
Updating the current CLI to allow users to test their existing queries against newer OpenSearch SQL plug-in versions before upgrading their OpenSearch clusters.
For example, a user is currently using version 2.19 may want to validate query compatibility with version 3.1 first.
By using this CLI tool, they can:
After users verify their queries are working as intended then they can updating their OpenSearch cluster version. This CLI can act as a safe testing environment, allowing smooth transitions between versions with confidence.
Moreover, developers can use this to test their own SQL plug-in implementation.
Command
--versionCommand
--local./gradlew assembleCommand
--remotegit clone --branch <branch_name> --single-branch <url/sql.git>sql.git. In this case, repo name issql. All git clone repo will be save inremotedirectoryCommand
--rebuildCommand
-sUnit tests are done
TODO:
Issues Resolved
N/A
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.