Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support uncorrelated scalar subquery after comparison operator in WHERE clause #14148

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

lancelly
Copy link
Contributor

@lancelly lancelly commented Nov 20, 2024

As titled.

@Cpaulyz
Copy link
Contributor

Cpaulyz commented Nov 20, 2024

What a spectacular work 🦾! Fabulous design and implementation, LGTM! ⚡️⚡️🔥

@lancelly lancelly marked this pull request as ready for review November 27, 2024 07:22
@THUMarkLau
Copy link
Contributor

I am utterly astounded and profoundly impressed by your recent Pull Request, "Support uncorrelated scalar subquery after comparison operator in WHERE clause," for the IoTDB project. Your exceptional work in enabling SQL subqueries within the WHERE clause is nothing short of groundbreaking and represents a monumental leap forward for the IoTDB community.

Your meticulous attention to detail and unparalleled expertise have culminated in a feature that not only enhances the functionality of IoTDB but also elevates its capabilities to new heights. The ability to support uncorrelated scalar subqueries after comparison operators is a testament to your deep understanding of complex database systems and your unwavering commitment to advancing open-source technology.

This contribution is poised to revolutionize the way users interact with IoTDB, providing them with unprecedented flexibility and power in their data queries. Your innovative approach and the elegance with which you have integrated this feature are truly commendable. It is evident that you have poured your heart and soul into this endeavor, and the results speak volumes about your dedication and brilliance.

In a world where technological advancements are often incremental, your work stands out as a beacon of ingenuity and excellence. You have not only addressed a significant gap in IoTDB's capabilities but have also set a new standard for what can be achieved through collaborative open-source development.

I am in awe of your talent and am incredibly proud to witness your contributions making such a profound impact. Your work will undoubtedly inspire others in the community to strive for greatness and push the boundaries of what is possible.

Congratulations on this remarkable achievement, and thank you for your invaluable contribution to the IoTDB project. The future of IoTDB is brighter because of your extraordinary efforts.

Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 25.80645% with 782 lines in your changes missing coverage. Please review.

Project coverage is 39.65%. Comparing base (05bc4fd) to head (951b50d).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
...ngine/plan/relational/planner/SubqueryPlanner.java 20.52% 120 Missing ⚠️
...rocess/join/SimpleNestedLoopCrossJoinOperator.java 0.00% 97 Missing ⚠️
...engine/plan/relational/planner/node/ApplyNode.java 0.00% 62 Missing ⚠️
...al/planner/optimizations/QueryCardinalityUtil.java 16.66% 50 Missing ⚠️
.../source/relational/TableFullOuterJoinOperator.java 0.00% 48 Missing ⚠️
...planner/optimizations/UnaliasSymbolReferences.java 39.13% 42 Missing ⚠️
...ator/source/relational/TableInnerJoinOperator.java 0.00% 37 Missing ⚠️
...tive/rule/TransformUncorrelatedSubqueryToJoin.java 37.73% 33 Missing ⚠️
...eryengine/plan/planner/TableOperatorGenerator.java 0.00% 32 Missing ⚠️
...ion/operator/process/EnforceSingleRowOperator.java 0.00% 31 Missing ⚠️
... and 30 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #14148      +/-   ##
============================================
+ Coverage     39.63%   39.65%   +0.01%     
  Complexity       71       71              
============================================
  Files          4242     4268      +26     
  Lines        270488   271435     +947     
  Branches      32806    32941     +135     
============================================
+ Hits         107215   107632     +417     
- Misses       163273   163803     +530     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


/*Use primitive types to avoid boxing and unboxing.*/

boolean lessThan(int left, int right);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also need boolean type?

case BLOB:
case TEXT:
case BOOLEAN:
default:
Copy link
Member

@Beyyes Beyyes Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOB,TEXT,BOOLEAN types are also needed to be support

Copy link
Contributor

@JackieTien97 JackieTien97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SimpleNestedLoopCrossJoinOperator, TableInnerJoinOperator and TableOperatorGenerator are not reviewed, should be reviewed by @Beyyes after the JoinKeyComaprator refactoring

}

@Override
/*@Override
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why need to delete the visitAggregation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what exactly the ExpressionExtractor used for?

Copy link
Contributor Author

@lancelly lancelly Nov 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

visitAggregation is not removed, its already been commented out previously. For now, ExpressionExtractor is used by SymbolExtractor to extractor symbols for optimizing phase.

Comment on lines +637 to +646
public List<PlanNode> visitEnforceSingleRow(EnforceSingleRowNode node, PlanContext context) {
List<PlanNode> childrenNodes = node.getChild().accept(this, context);
OrderingScheme childOrdering = nodeOrderingMap.get(childrenNodes.get(0).getPlanNodeId());
if (childOrdering != null) {
nodeOrderingMap.put(node.getPlanNodeId(), childOrdering);
}

node.setChild(mergeChildrenViaCollectOrMergeSort(childOrdering, childrenNodes));
return Collections.singletonList(node);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add EnforceSingleRowNode for each children and then call mergeChildrenViaCollectOrMergeSort, and then followed by a EnforceSingleRowNode may be better.

* <p>INNER - does not return any row for input row when subquery relation is empty LEFT - does
* return input completed with NULL values when subquery relation is empty
*/
public class CorrelatedJoinNode extends TwoChildProcessNode {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may need to explain what's the difference between CorrelatedJoinNode and ApplyNode, and how an unCorrelated Join be transformed from CorrelatedJoinNode or ApplyNode to a JoinNode

import static org.apache.iotdb.db.queryengine.plan.relational.planner.PlanNodeSearcher.searchFrom;
import static org.apache.iotdb.rpc.TSStatusCode.SEMANTIC_ERROR;

public class CheckSubqueryNodesAreRewritten implements PlanOptimizer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a little bit confused here, what plan nodes will correlated subqueries use? not ApplyNode and CorrelatedJoinNode?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Join and SemiJoin are often used by correlated subqueries.

plannerContext,
ruleStats,
ImmutableSet.of(
new RemoveRedundantEnforceSingleRowNode(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may need to add it previous IterativeOptimizer redundantly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked this, will add it in an preprevious IterativeOptimizer redundantly.

ImmutableSet.of(
new RemoveRedundantEnforceSingleRowNode(),
new TransformUncorrelatedSubqueryToJoin())),
new CheckSubqueryNodesAreRewritten(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In trino, it seems that we also need to add another simplifyOptimizer after it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I'm not certain its related to subquery. I will add simplifyOptimizer after this rule.

Comment on lines 188 to 191
// public Range<Long> visitTopN(TopNNode node, Void context)
// {
// return applyLimit(node.getSource(), node.getCount());
// }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we also have TopKNode

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


import org.apache.tsfile.utils.Binary;

public class AscJoinKeyComparator implements JoinKeyComparator {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Comparator shouldn't be this way, we should only have

  1. lessThan(TsBlock left, int leftRowIndex, TsBlock right, int rightRowIndex)
  2. equalsTo(TsBlock left, int leftRowIndex, TsBlock right, int rightRowIndex)
  3. lessThanOrEqual(TsBlock left, int leftRowIndex, TsBlock right, int rightRowIndex)

in interface, and asc/desc implementation class for each type.

Comment on lines 436 to 437
node.setLeftChild(mergeChildrenViaCollectOrMergeSort(null, leftChildrenNodes));
node.setRightChild(mergeChildrenViaCollectOrMergeSort(null, rightChildrenNodes));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

directly passing null as OrderingScheme? should use the left and right child's orderingschema

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. Fixed.

Copy link

sonarcloud bot commented Nov 29, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants