Data fusion changes - #1
Conversation
--------- Signed-off-by: Kai Huang <[email protected]>
* PPL fillnull command enhancement Signed-off-by: Kai Huang <[email protected]> # Conflicts: # integ-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.java * add to searchableKeyWord Signed-off-by: Kai Huang <[email protected]> * fixes Signed-off-by: Kai Huang <[email protected]> * fix CI Signed-off-by: Kai Huang <[email protected]> * update error message handling Signed-off-by: Kai Huang <[email protected]> * formatting Signed-off-by: Kai Huang <[email protected]> * put file back Signed-off-by: Kai Huang <[email protected]> * removal Signed-off-by: Kai Huang <[email protected]> * update doc Signed-off-by: Kai Huang <[email protected]> * update doc Signed-off-by: Kai Huang <[email protected]> * add IT Signed-off-by: Kai Huang <[email protected]> --------- Signed-off-by: Kai Huang <[email protected]>
…ct#4442) * Add ignorePrometheus flag Signed-off-by: Peng Huo <[email protected]> * support -DignorePrometheus in integTest and docTest Signed-off-by: Peng Huo <[email protected]> * Update Signed-off-by: Peng Huo <[email protected]> * Update Signed-off-by: Peng Huo <[email protected]> --------- Signed-off-by: Peng Huo <[email protected]>
Fixed typo: evenstats --> eventstats Signed-off-by: Alexey Temnikov <[email protected]>
…ches (opensearch-project#4025) * Update delete_backport_branch workflow to include release-chores branches Signed-off-by: Riley Jerger <[email protected]> * Update delete_backport_branch workflow to use github-script with proper permissions Signed-off-by: Riley Jerger <[email protected]> --------- Signed-off-by: Riley Jerger <[email protected]>
…nsearch-project#4454) * Resolve concurrency issue Signed-off-by: Louis Chu <[email protected]> * Update fix Signed-off-by: Louis Chu <[email protected]> * Add UT Signed-off-by: Louis Chu <[email protected]> * Revise comments Signed-off-by: Louis Chu <[email protected]> * Fix style Signed-off-by: Louis Chu <[email protected]> --------- Signed-off-by: Louis Chu <[email protected]>
* Refactor qualified name resolution in PPL Signed-off-by: Tomoyuki Morita <[email protected]> * Add tests Signed-off-by: Tomoyuki Morita <[email protected]> * fix naming Signed-off-by: Tomoyuki Morita <[email protected]> * Minor fix Signed-off-by: Tomoyuki Morita <[email protected]> * Fix test failure Signed-off-by: Tomoyuki Morita <[email protected]> --------- Signed-off-by: Tomoyuki Morita <[email protected]>
…e join criteria (opensearch-project#4474) Signed-off-by: Lantao Jin <[email protected]>
…ITs (opensearch-project#4462) * Use Guice.createInjector Signed-off-by: Peng Huo <[email protected]> * Update Signed-off-by: Peng Huo <[email protected]> --------- Signed-off-by: Peng Huo <[email protected]>
Signed-off-by: Peng Huo <[email protected]>
* Add mvappend function for Calcite PPL Signed-off-by: Tomoyuki Morita <[email protected]> * Fix annonymizer test Signed-off-by: Tomoyuki Morita <[email protected]> * Fix IT Signed-off-by: Tomoyuki Morita <[email protected]> * Minor fix Signed-off-by: Tomoyuki Morita <[email protected]> * Fix type coercion issue Signed-off-by: Tomoyuki Morita <[email protected]> * Fix test Signed-off-by: Tomoyuki Morita <[email protected]> --------- Signed-off-by: Tomoyuki Morita <[email protected]>
* Fix missing keywordsCanBeId Signed-off-by: Lantao Jin <[email protected]> * revert partially Signed-off-by: Lantao Jin <[email protected]> --------- Signed-off-by: Lantao Jin <[email protected]>
…oject#4475) Signed-off-by: Songkan Tang <[email protected]>
…pensearch-project#4413) * Fallback to sub-aggregation if composite aggregation doesn't support Signed-off-by: Heng Qian <[email protected]> * merging main Signed-off-by: Heng Qian <[email protected]> * Address comments Signed-off-by: Heng Qian <[email protected]> * Address comments Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]>
…search-project#4440) --------- Signed-off-by: Peng Huo <[email protected]>
Signed-off-by: Tomoyuki Morita <[email protected]>
…pensearch-project#4520) Signed-off-by: Yuanchun Shen <[email protected]>
* Fix mapping after aggregation push down Signed-off-by: Heng Qian <[email protected]> * Fix IT and UT Signed-off-by: Heng Qian <[email protected]> * address comments Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]>
* Add MAP_CONCAT internal function Signed-off-by: Tomoyuki Morita <[email protected]> * Minor fix Signed-off-by: Tomoyuki Morita <[email protected]> --------- Signed-off-by: Tomoyuki Morita <[email protected]> Signed-off-by: Tomoyuki MORITA <[email protected]>
…-project#4464) Add per_second() support to the timechart command by implementing Option 3 (Eval Transformation). --------- Signed-off-by: Chen Dai <[email protected]>
…opensearch-project#4501) * Add configurable sytem limitations for subsearch and join command Signed-off-by: Lantao Jin <[email protected]> * Fix IT Signed-off-by: Lantao Jin <[email protected]> * typo Signed-off-by: Lantao Jin <[email protected]> * fix IT Signed-off-by: Lantao Jin <[email protected]> * remove rollback in doc Signed-off-by: Lantao Jin <[email protected]> * address comments Signed-off-by: Lantao Jin <[email protected]> * fix typo Signed-off-by: Lantao Jin <[email protected]> * Fix IT Signed-off-by: Lantao Jin <[email protected]> --------- Signed-off-by: Lantao Jin <[email protected]>
…pensearch-project#4534) * [FollowUp] Set 0 and negative value of subsearch.maxout as unlimited Signed-off-by: Lantao Jin <[email protected]> * fix doctest Signed-off-by: Lantao Jin <[email protected]> * Fix conflicts Signed-off-by: Lantao Jin <[email protected]> --------- Signed-off-by: Lantao Jin <[email protected]>
* fix percentile bug Signed-off-by: xinyual <[email protected]> * add IT Signed-off-by: xinyual <[email protected]> * optimize it Signed-off-by: xinyual <[email protected]> --------- Signed-off-by: xinyual <[email protected]>
…opensearch-project#4522) * Including metadata fields type when doing agg/filter script push down Signed-off-by: Heng Qian <[email protected]> * Fix IT Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]>
…ch-project#4541) Signed-off-by: Lantao Jin <[email protected]>
Updates CalcitePPLClickbenchITs
Signed-off-by: expani <[email protected]>
Signed-off-by: expani <[email protected]>
Signed-off-by: expani <[email protected]>
Signed-off-by: Sandesh Kumar <[email protected]>
Signed-off-by: Vinay Krishna Pudyodu <[email protected]>
This reverts commit eaab86f.
Signed-off-by: Marc Handalian <[email protected]>
…lan Adding e2e test workflow add e2e test workflow to sql
* Added support for Timestamp fields in filter Signed-off-by: Vinay Krishna Pudyodu <[email protected]> * Updated the condition in TypeConverted to handle timestamp udt Signed-off-by: Vinay Krishna Pudyodu <[email protected]> * Modifed the updateTimeStampFunction to handle recursively and updated checker method for timestamp udt Signed-off-by: Vinay Krishna Pudyodu <[email protected]> --------- Signed-off-by: Vinay Krishna Pudyodu <[email protected]>
* Add assertions on all 43 queries against expected results from 3.4. Updated test to categorize into passing/failing (non200) and failing with 200. Signed-off-by: Marc Handalian <[email protected]> * add CalcitePPLClickBenchIT.testDataFusion to e2e test workflow Signed-off-by: Marc Handalian <[email protected]> * Update dataset to have hits on all queries, update expected fixtures. Signed-off-by: Marc Handalian <[email protected]> --------- Signed-off-by: Marc Handalian <[email protected]>
… UDTs (opensearch-project#12) Signed-off-by: Vinay Krishna Pudyodu <[email protected]>
count changes
Signed-off-by: Marc Handalian <[email protected]>
…plan update gh workflow to display pass/fail output and update supported list
…ing on. Signed-off-by: Marc Handalian <[email protected]>
…eries Signed-off-by: Marc Handalian <[email protected]>
…plan update index to multi shard and update ignored set to ignore fetch queries
Signed-off-by: Marc Handalian <[email protected]>
…plan ignore query 4
…unctions (opensearch-project#16) * Added common visitor to update timestamp and extract functions in relnode Signed-off-by: Vinay Krishna Pudyodu <[email protected]> * Added mapping for regexp_replace Signed-off-by: Vinay Krishna Pudyodu <[email protected]> * Removed some unnecessary lines Signed-off-by: Vinay Krishna Pudyodu <[email protected]> * Updated the mapStringToTimeUnitRange method with all supported cases Signed-off-by: Vinay Krishna Pudyodu <[email protected]> * Refactors for SPAN and LIKE function preprocess Signed-off-by: Vinay Krishna Pudyodu <[email protected]> * Removed the resource dir which added in earlier commit Signed-off-by: Vinay Krishna Pudyodu <[email protected]> * Added UDF in substrait for date_part and date_format Signed-off-by: Vinay Krishna Pudyodu <[email protected]> * Removed isExtractFunction Signed-off-by: Vinay Krishna Pudyodu <[email protected]> --------- Signed-off-by: Vinay Krishna Pudyodu <[email protected]>
Signed-off-by: Vinay Krishna Pudyodu <[email protected]>
…pensearch-project#15) * avg fix field mapping Signed-off-by: Sandesh Kumar <[email protected]> * fix dependency resolution Signed-off-by: Sandesh Kumar <[email protected]> --------- Signed-off-by: Sandesh Kumar <[email protected]>
Summary of ChangesHello @sandeshkr419, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the PPL query engine's capabilities by introducing a new Unified Query API and several powerful commands and functions. The changes focus on improving data fusion, query planning, and optimization, while also addressing type handling, error reporting, and resource management. These updates aim to provide a more robust and versatile analytical experience for users. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Signed-off-by: Sandesh Kumar <[email protected]>
There was a problem hiding this comment.
Code Review
This pull request introduces a significant set of changes, primarily focused on a new "Data Fusion" or "Unified Query" API, along with a major Calcite version upgrade and the introduction of Substrait dependencies. It also adds several new PPL commands (multisearch, replace, streamstats) and functions, and includes numerous refactorings and documentation updates.
My review has identified a few critical and high-severity issues that should be addressed before merging:
- A hardcoded change in
QueryService.javaenables the Calcite engine for all query types, including SQL, which is not yet fully supported and could cause breakages. - The mapping of
IPandbinarydata types toBIGINTinOpenSearchTypeFactory.javais a breaking change that will affect functions relying on these types. - There is a minor duplication in the root
build.gradlefile that should be cleaned up for maintainability.
Overall, the introduction of the Unified Query API and the refactoring efforts (like QualifiedNameResolver and SchemaUnifier) are positive steps towards a more robust and extensible query engine. The documentation updates are also very thorough and helpful. Once the identified issues are resolved, this will be a strong contribution.
| private boolean shouldUseCalcite(QueryType queryType) { | ||
| return isCalciteEnabled(settings) && queryType == QueryType.PPL; | ||
| return true;//isCalciteEnabled(settings) && queryType == QueryType.PPL; | ||
| } |
There was a problem hiding this comment.
The shouldUseCalcite method is hardcoded to return true. Based on the commented-out code and the comment // Calcite is not available for SQL query now., it seems this change is intended for development or testing purposes. Merging this will enable the Calcite engine for all query types, including SQL, which could lead to unexpected behavior or breakages for SQL queries. This should be reverted to the original logic before merging.
| private boolean shouldUseCalcite(QueryType queryType) { | |
| return isCalciteEnabled(settings) && queryType == QueryType.PPL; | |
| return true;//isCalciteEnabled(settings) && queryType == QueryType.PPL; | |
| } | |
| private boolean shouldUseCalcite(QueryType queryType) { | |
| return isCalciteEnabled(settings) && queryType == QueryType.PPL; | |
| } |
| return TYPE_FACTORY.createSqlType(SqlTypeName.BIGINT, nullable); | ||
| // return TYPE_FACTORY.createUDT(ExprUDT.EXPR_IP, nullable); |
There was a problem hiding this comment.
The IP data type is now being mapped to BIGINT instead of the user-defined EXPR_IP type. The comment suggests this is for Substrait compatibility. However, this is a breaking change that will likely cause functions that operate on IP addresses, such as CIDRMATCH, to fail because they will receive a BIGINT instead of the expected IP type. This change should be made conditional or handled in a way that doesn't break existing functionality for non-Substrait use cases.
| return TYPE_FACTORY.createSqlType(SqlTypeName.BIGINT, nullable); | |
| // return TYPE_FACTORY.createUDT(ExprUDT.EXPR_IP, nullable); | |
| return TYPE_FACTORY.createUDT(ExprUDT.EXPR_IP, nullable); |
| // return TYPE_FACTORY.createUDT(ExprUDT.EXPR_BINARY, nullable); | ||
| } else if (fieldType.legacyTypeName().equalsIgnoreCase("timestamp")) { |
There was a problem hiding this comment.
The legacy binary data type is now being mapped to BIGINT instead of the user-defined EXPR_BINARY type. Similar to the change for the IP type, this is a breaking change motivated by Substrait compatibility. This could cause issues for any functionality that relies on the binary type. This change should be conditional to avoid breaking existing behavior.
| // return TYPE_FACTORY.createUDT(ExprUDT.EXPR_BINARY, nullable); | |
| } else if (fieldType.legacyTypeName().equalsIgnoreCase("timestamp")) { | |
| return TYPE_FACTORY.createUDT(ExprUDT.EXPR_BINARY, nullable); |
| return TYPE_FACTORY.createSqlType(SqlTypeName.BIGINT, nullable); | ||
| // return TYPE_FACTORY.createUDT(ExprUDT.EXPR_IP, nullable); |
There was a problem hiding this comment.
The legacy ip data type is also being mapped to BIGINT. This is a breaking change for the same reasons as the IP enum case. This should be reverted to use the EXPR_IP UDT to maintain compatibility.
| return TYPE_FACTORY.createSqlType(SqlTypeName.BIGINT, nullable); | |
| // return TYPE_FACTORY.createUDT(ExprUDT.EXPR_IP, nullable); | |
| return TYPE_FACTORY.createUDT(ExprUDT.EXPR_IP, nullable); |
| resolutionStrategy.force 'org.apache.calcite.avatica:avatica-core:1.26.0' | ||
| resolutionStrategy.force 'org.slf4j:slf4j-api:2.0.13' |
Signed-off-by: Sandesh Kumar <[email protected]>
Description
Version forced to 3.3.0.
Testing:
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
Check List
--signoffor-s.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.