Skip to content

Commit 1772ef9

Browse files
committed
Fix interval semantic mismatch between sql and ppl
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
1 parent 611efca commit 1772ef9

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
@@ -47,10 +47,11 @@
4747
import org.opensearch.sql.calcite.CalcitePlanContext;
4848
import org.opensearch.sql.calcite.CalciteRelNodeVisitor;
4949
import org.opensearch.sql.calcite.OpenSearchSchema;
50-
import org.opensearch.sql.calcite.PplRelToSqlConverter;
5150
import org.opensearch.sql.calcite.SysLimit;
5251
import org.opensearch.sql.calcite.plan.LogicalSystemLimit;
5352
import org.opensearch.sql.calcite.plan.LogicalSystemLimit.SystemLimitType;
53+
import org.opensearch.sql.calcite.validate.PplRelToSqlNodeConverter;
54+
import org.opensearch.sql.calcite.validate.PplRelToSqlRelShuttle;
5455
import org.opensearch.sql.common.response.ResponseListener;
5556
import org.opensearch.sql.common.setting.Settings;
5657
import org.opensearch.sql.datasource.DataSourceService;
@@ -73,8 +74,8 @@ public class QueryService {
7374
private final Planner planner;
7475
private DataSourceService dataSourceService;
7576
private Settings settings;
76-
private static final PplRelToSqlConverter converter =
77-
new PplRelToSqlConverter(MysqlSqlDialect.DEFAULT);
77+
private static final PplRelToSqlNodeConverter converter =
78+
new PplRelToSqlNodeConverter(MysqlSqlDialect.DEFAULT);
7879

7980
@Getter(lazy = true)
8081
private final CalciteRelNodeVisitor relNodeVisitor = new CalciteRelNodeVisitor(dataSourceService);
@@ -295,8 +296,11 @@ public LogicalPlan analyze(UnresolvedPlan plan, QueryType queryType) {
295296
* @return the validated (and potentially modified) relation node
296297
*/
297298
private RelNode validate(RelNode relNode, CalcitePlanContext context) {
299+
// Fix interval literals before conversion to SQL
300+
RelNode sqlRelNode = relNode.accept(new PplRelToSqlRelShuttle(context.rexBuilder, true));
301+
298302
// Convert RelNode to SqlNode for validation
299-
SqlImplementor.Result result = converter.visitRoot(relNode);
303+
SqlImplementor.Result result = converter.visitRoot(sqlRelNode);
300304
SqlNode root = result.asStatement();
301305

302306
// Rewrite SqlNode to remove database qualifiers
@@ -318,20 +322,14 @@ public SqlNode visit(SqlIdentifier id) {
318322
SqlValidator validator = context.getValidator();
319323
if (rewritten != null) {
320324
try {
321-
String before = rewritten.toString();
322-
// rewritten will be modified in-place by validation
325+
log.debug("Before validation: {}", rewritten);
323326
validator.validate(rewritten);
324327
log.debug("After validation: {}", rewritten);
325-
String after = rewritten.toString();
326-
if (before.equals(after)) {
327-
// If the rewritten SQL node is not modified, return the original RelNode as is
328-
return relNode;
329-
}
330328
} catch (CalciteContextException e) {
331329
throw new ExpressionEvaluationException(e.getMessage(), e);
332330
}
333331
} else {
334-
log.debug("Failed to rewrite the SQL node before validation: {}", root);
332+
log.info("Failed to rewrite the SQL node before validation: {}", root);
335333
return relNode;
336334
}
337335

@@ -348,8 +346,8 @@ public SqlNode visit(SqlIdentifier id) {
348346
cluster,
349347
StandardConvertletTable.INSTANCE,
350348
SqlToRelConverter.config());
351-
RelRoot validatedRelRoot = sql2rel.convertQuery(rewritten, true, true);
352-
return validatedRelRoot.rel;
349+
RelRoot validatedRelRoot = sql2rel.convertQuery(rewritten, false, true);
350+
return validatedRelRoot.rel.accept(new PplRelToSqlRelShuttle(context.rexBuilder, false));
353351
}
354352

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

0 commit comments

Comments
 (0)