Skip to content

Commit 2984c52

Browse files
committed
Fix interval semantic mismatch between sql and ppl
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
1 parent 8d0c631 commit 2984c52

3 files changed

Lines changed: 73 additions & 17 deletions

File tree

core/src/main/java/org/opensearch/sql/calcite/PplRelToSqlConverter.java renamed to core/src/main/java/org/opensearch/sql/calcite/validate/PplRelToSqlNodeConverter.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
* SPDX-License-Identifier: Apache-2.0
44
*/
55

6-
package org.opensearch.sql.calcite;
6+
package org.opensearch.sql.calcite.validate;
77

88
import org.apache.calcite.rel.rel2sql.RelToSqlConverter;
99
import org.apache.calcite.sql.SqlDialect;
@@ -15,8 +15,8 @@
1515
* <p>This converter is used during the validation phase to convert RelNode back to SqlNode for
1616
* validation and type checking using Calcite's SqlValidator.
1717
*
18-
* <p>Currently, we haven't implemented any specific changes to it, just leaving it for future
19-
* extension.
18+
* <p>Note: Interval literal issues are handled by preprocessing with {@link
19+
* IntervalLiteralFixShuttle} before conversion.
2020
*/
2121
public class PplRelToSqlConverter extends RelToSqlConverter {
2222
/**
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
/*
2+
* Copyright OpenSearch Contributors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package org.opensearch.sql.calcite.validate;
7+
8+
import java.math.BigDecimal;
9+
import org.apache.calcite.avatica.util.TimeUnit;
10+
import org.apache.calcite.rel.RelNode;
11+
import org.apache.calcite.rel.RelShuttleImpl;
12+
import org.apache.calcite.rex.RexBuilder;
13+
import org.apache.calcite.rex.RexLiteral;
14+
import org.apache.calcite.rex.RexNode;
15+
import org.apache.calcite.rex.RexShuttle;
16+
import org.apache.calcite.sql.SqlIntervalQualifier;
17+
18+
/**
19+
* A RelShuttle that recursively visits all RelNodes and their RexNode expressions to fix interval
20+
* literals before/after SQL conversion.
21+
*
22+
* <p>This shuttle extends RelShuttleImpl to ensure it visits the entire RelNode tree recursively,
23+
* applying the interval literal fixes at each node.
24+
*/
25+
public class PplRelToSqlRelShuttle extends RelShuttleImpl {
26+
private final RexShuttle rexShuttle;
27+
28+
public PplRelToSqlRelShuttle(RexBuilder rexBuilder, boolean forward) {
29+
this.rexShuttle =
30+
new RexShuttle() {
31+
@Override
32+
public RexNode visitLiteral(RexLiteral literal) {
33+
SqlIntervalQualifier qualifier = literal.getType().getIntervalQualifier();
34+
if (qualifier == null) {
35+
return literal;
36+
}
37+
BigDecimal value = literal.getValueAs(BigDecimal.class);
38+
if (value == null) {
39+
return literal;
40+
}
41+
TimeUnit unit = qualifier.getUnit();
42+
BigDecimal newValue =
43+
forward
44+
? value.multiply(unit.multiplier)
45+
: value.divideToIntegralValue(unit.multiplier);
46+
return rexBuilder.makeIntervalLiteral(newValue, qualifier);
47+
}
48+
};
49+
}
50+
51+
@Override
52+
protected RelNode visitChild(RelNode parent, int i, RelNode child) {
53+
// First visit the child recursively
54+
RelNode newChild = super.visitChild(parent, i, child);
55+
// Then apply the RexShuttle to the child's expressions
56+
return newChild.accept(rexShuttle);
57+
}
58+
}

core/src/main/java/org/opensearch/sql/executor/QueryService.java

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,11 @@
5151
import org.opensearch.sql.calcite.CalcitePlanContext;
5252
import org.opensearch.sql.calcite.CalciteRelNodeVisitor;
5353
import org.opensearch.sql.calcite.OpenSearchSchema;
54-
import org.opensearch.sql.calcite.PplRelToSqlConverter;
5554
import org.opensearch.sql.calcite.SysLimit;
5655
import org.opensearch.sql.calcite.plan.LogicalSystemLimit;
5756
import org.opensearch.sql.calcite.plan.LogicalSystemLimit.SystemLimitType;
57+
import org.opensearch.sql.calcite.validate.PplRelToSqlNodeConverter;
58+
import org.opensearch.sql.calcite.validate.PplRelToSqlRelShuttle;
5859
import org.opensearch.sql.common.response.ResponseListener;
5960
import org.opensearch.sql.common.setting.Settings;
6061
import org.opensearch.sql.datasource.DataSourceService;
@@ -80,8 +81,8 @@ public class QueryService {
8081
private final Planner planner;
8182
private DataSourceService dataSourceService;
8283
private Settings settings;
83-
private static final PplRelToSqlConverter converter =
84-
new PplRelToSqlConverter(MysqlSqlDialect.DEFAULT);
84+
private static final PplRelToSqlNodeConverter converter =
85+
new PplRelToSqlNodeConverter(MysqlSqlDialect.DEFAULT);
8586

8687
@Getter(lazy = true)
8788
private final CalciteRelNodeVisitor relNodeVisitor = new CalciteRelNodeVisitor(dataSourceService);
@@ -314,8 +315,11 @@ public LogicalPlan analyze(UnresolvedPlan plan, QueryType queryType) {
314315
* @return the validated (and potentially modified) relation node
315316
*/
316317
private RelNode validate(RelNode relNode, CalcitePlanContext context) {
318+
// Fix interval literals before conversion to SQL
319+
RelNode sqlRelNode = relNode.accept(new PplRelToSqlRelShuttle(context.rexBuilder, true));
320+
317321
// Convert RelNode to SqlNode for validation
318-
SqlImplementor.Result result = converter.visitRoot(relNode);
322+
SqlImplementor.Result result = converter.visitRoot(sqlRelNode);
319323
SqlNode root = result.asStatement();
320324

321325
// Rewrite SqlNode to remove database qualifiers
@@ -337,20 +341,14 @@ public SqlNode visit(SqlIdentifier id) {
337341
SqlValidator validator = context.getValidator();
338342
if (rewritten != null) {
339343
try {
340-
String before = rewritten.toString();
341-
// rewritten will be modified in-place by validation
344+
log.debug("Before validation: {}", rewritten);
342345
validator.validate(rewritten);
343346
log.debug("After validation: {}", rewritten);
344-
String after = rewritten.toString();
345-
if (before.equals(after)) {
346-
// If the rewritten SQL node is not modified, return the original RelNode as is
347-
return relNode;
348-
}
349347
} catch (CalciteContextException e) {
350348
throw new ExpressionEvaluationException(e.getMessage(), e);
351349
}
352350
} else {
353-
log.debug("Failed to rewrite the SQL node before validation: {}", root);
351+
log.info("Failed to rewrite the SQL node before validation: {}", root);
354352
return relNode;
355353
}
356354

@@ -367,8 +365,8 @@ public SqlNode visit(SqlIdentifier id) {
367365
cluster,
368366
StandardConvertletTable.INSTANCE,
369367
SqlToRelConverter.config());
370-
RelRoot validatedRelRoot = sql2rel.convertQuery(rewritten, true, true);
371-
return validatedRelRoot.rel;
368+
RelRoot validatedRelRoot = sql2rel.convertQuery(rewritten, false, true);
369+
return validatedRelRoot.rel.accept(new PplRelToSqlRelShuttle(context.rexBuilder, false));
372370
}
373371

374372
/** Translate {@link LogicalPlan} to {@link PhysicalPlan}. */

0 commit comments

Comments
 (0)