JackieTien97 commented on code in PR #15960:
URL: https://github.com/apache/iotdb/pull/15960#discussion_r2212223532
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/analyzer/predicate/PredicateCombineIntoTableScanChecker.java:
##########
@@ -41,17 +41,23 @@
import static
org.apache.iotdb.db.queryengine.plan.relational.analyzer.predicate.PredicatePushIntoScanChecker.isLiteral;
import static
org.apache.iotdb.db.queryengine.plan.relational.analyzer.predicate.PredicatePushIntoScanChecker.isSymbolReference;
+import static
org.apache.iotdb.db.queryengine.plan.relational.planner.ir.GlobalTimePredicateExtractVisitor.isExtractTimeColumn;
public class PredicateCombineIntoTableScanChecker extends
PredicateVisitor<Boolean, Void> {
private final Set<String> measurementColumns;
+ private final String timeColumnName;
Review Comment:
no need to add this field?, timeColumnName has already been put in
`measurementColumns`.
<img width="1426" height="1460" alt="image"
src="https://github.com/user-attachments/assets/0fdba63e-567a-4b5c-98ae-278abbf63c06";
/>
maybe you can add some comments about `measurementColumns` and change it
name from `measurementColumns` to `timeAndFieldsColumnNames`
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/planner/IrTypeAnalyzer.java:
##########
@@ -306,6 +307,11 @@ protected Type
visitArithmeticUnary(ArithmeticUnaryExpression node, Context cont
return setExpressionType(node, process(node.getValue(), context));
}
+ @Override
+ protected Type visitExtract(Extract node, Context context) {
+ return setExpressionType(node, INT64);
Review Comment:
```suggestion
process(node.getExpression(), context);
return setExpressionType(node, INT64);
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/planner/ir/GlobalTimePredicateExtractVisitor.java:
##########
@@ -269,9 +271,18 @@ public static boolean isTimeColumn(Expression e, String
timeColumnName) {
&& ((SymbolReference) e).getName().equalsIgnoreCase(timeColumnName);
}
+ public static boolean isExtractTimeColumn(Expression e, String
timeColumnName) {
+ return e instanceof Extract
+ && ((SymbolReference) ((Extract) e).getExpression())
Review Comment:
what if express of Extract is a constant, like `where EXTRACT(HOUR FROM
2025/07/08 01:18:51.123) =1`
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/analyzer/ExpressionAnalyzer.java:
##########
@@ -1555,6 +1557,22 @@ protected Type visitParameter(Parameter node,
StackableAstVisitorContext<Context
return setExpressionType(node, resultType);
}
+ @Override
+ protected Type visitExtract(Extract node,
StackableAstVisitorContext<Context> context) {
+ if (node.getExpression() instanceof LongLiteral) {
+ // Don't visit child here to avoid setting its Type to INT32
+ setExpressionType(node.getExpression(), INT64);
Review Comment:
duplicated with the last return statement?
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/analyzer/predicate/PredicateCombineIntoTableScanChecker.java:
##########
@@ -115,14 +121,19 @@ protected Boolean
visitLogicalExpression(LogicalExpression node, Void context) {
@Override
protected Boolean visitComparisonExpression(ComparisonExpression node, Void
context) {
return (isMeasurementColumn(node.getLeft()) && isLiteral(node.getRight()))
- || (isMeasurementColumn(node.getRight()) && isLiteral(node.getLeft()));
+ || (isMeasurementColumn(node.getRight()) && isLiteral(node.getLeft()))
+ || (isExtractTimeColumn(node.getLeft(), timeColumnName) &&
isLiteral(node.getRight()))
Review Comment:
even if extract from a field, we should also push it down into tableScan
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/planner/optimizations/PushPredicateIntoTableScan.java:
##########
@@ -530,7 +531,7 @@ private SplitExpression splitPredicate(DeviceTableScanNode
node, Expression pred
if (PredicatePushIntoMetadataChecker.check(idOrAttributeColumnNames,
expression)) {
metadataExpressions.add(expression);
} else if (PredicateCombineIntoTableScanChecker.check(
- measurementColumnNames, expression)) {
+ measurementColumnNames, timeColumnName, expression)) {
Review Comment:
```suggestion
measurementColumnNames, expression)) {
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/planner/optimizations/PushPredicateIntoTableScan.java:
##########
@@ -543,7 +544,8 @@ private SplitExpression splitPredicate(DeviceTableScanNode
node, Expression pred
if (PredicatePushIntoMetadataChecker.check(idOrAttributeColumnNames,
predicate)) {
metadataExpressions.add(predicate);
- } else if
(PredicateCombineIntoTableScanChecker.check(measurementColumnNames, predicate))
{
+ } else if (PredicateCombineIntoTableScanChecker.check(
+ measurementColumnNames, timeColumnName, predicate)) {
Review Comment:
```suggestion
measurementColumnNames, predicate)) {
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/planner/ir/GlobalTimePredicateExtractVisitor.java:
##########
@@ -269,9 +271,18 @@ public static boolean isTimeColumn(Expression e, String
timeColumnName) {
&& ((SymbolReference) e).getName().equalsIgnoreCase(timeColumnName);
}
+ public static boolean isExtractTimeColumn(Expression e, String
timeColumnName) {
+ return e instanceof Extract
+ && ((SymbolReference) ((Extract) e).getExpression())
Review Comment:
have a try using `IrExpressionInterpreter` to do the constant fold
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
