Skip to content

Coerce temporal operands in IN and BETWEEN when leastRestrictive finds no common type#5513

Open
LantaoJin wants to merge 1 commit into
opensearch-project:mainfrom
LantaoJin:fix/ppl-temporal-in-between
Open

Coerce temporal operands in IN and BETWEEN when leastRestrictive finds no common type#5513
LantaoJin wants to merge 1 commit into
opensearch-project:mainfrom
LantaoJin:fix/ppl-temporal-in-between

Conversation

@LantaoJin
Copy link
Copy Markdown
Member

@LantaoJin LantaoJin commented Jun 4, 2026

Description

visitIn and visitBetween call leastRestrictive directly instead of going through the CoercionUtils path that comparison operators use. leastRestrictive returns no common type when temporal operands are represented differently — e.g. a standard Calcite TIMESTAMP field against EXPR_DATE UDT bounds/values (ts between date('...') and date('...'), ts in (DATE '...', ...)) — so both predicates rejected such queries with "expression types are incompatible" even though comparisons coerce the same mix.

Both now fall back to CoercionUtils.widenArguments (the comparison-operator path) when leastRestrictive yields null, scoped to all-temporal operands so genuinely incompatible mixes (e.g. age between '35' and 38.5) still raise SemanticCheckException. Enables the previously-undiscovered testDateBetween (was missing its @test annotation) and adds testDateIn plus a CoercionUtils unit test covering the plain-TIMESTAMP + EXPR_DATE mix.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • New PPL command checklist all confirmed.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff or -s.
  • Public documentation issue/PR created.

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.

…s no common type

visitIn and visitBetween call leastRestrictive directly instead of going through the
CoercionUtils path that comparison operators use. leastRestrictive returns no common
type when temporal operands are represented differently — e.g. a standard Calcite
TIMESTAMP field against EXPR_DATE UDT bounds/values (`ts between date('...') and
date('...')`, `ts in (DATE '...', ...)`) — so both predicates rejected such queries
with "expression types are incompatible" even though comparisons coerce the same mix.

Both now fall back to CoercionUtils.widenArguments (the comparison-operator path) when
leastRestrictive yields null, scoped to all-temporal operands so genuinely incompatible
mixes (e.g. `age between '35' and 38.5`) still raise SemanticCheckException. Enables the
previously-undiscovered testDateBetween (was missing its @test annotation) and adds
testDateIn plus a CoercionUtils unit test covering the plain-TIMESTAMP + EXPR_DATE mix.

Signed-off-by: Lantao Jin <ltjin@amazon.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 4, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Possible Issue

In visitIn, when widenTemporalOperands returns non-null, the widened field is extracted as the last element and widened values as all preceding elements. However, the input list is constructed with valueList first, then field appended. This ordering mismatch means the widened field is incorrectly taken from what was originally a value, and vice versa. The correct extraction should match the input order: field at the end, values before it.

RexNode widenedField = widened.get(widened.size() - 1);
List<RexNode> widenedValues = widened.subList(0, widened.size() - 1);
return context.rexBuilder.makeIn(widenedField, widenedValues);

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 4, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Maintain consistent operand ordering

The order of operands matters when constructing the IN expression. The field should
be the first element, followed by the value list, to maintain consistency with the
original structure where field is tested against valueList. Currently, the field is
appended last and extracted from the end, which works but is less intuitive.

core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [244-251]

-List<RexNode> operands = new ArrayList<>(valueList);
+List<RexNode> operands = new ArrayList<>();
 operands.add(field);
+operands.addAll(valueList);
 List<RexNode> widened = widenTemporalOperands(context, operands);
 if (widened != null) {
-  RexNode widenedField = widened.get(widened.size() - 1);
-  List<RexNode> widenedValues = widened.subList(0, widened.size() - 1);
+  RexNode widenedField = widened.get(0);
+  List<RexNode> widenedValues = widened.subList(1, widened.size());
   return context.rexBuilder.makeIn(widenedField, widenedValues);
 }
Suggestion importance[1-10]: 4

__

Why: While the suggestion correctly identifies that operand ordering could be more intuitive, the current implementation is functionally correct and works as intended. The change would improve code readability slightly but has minimal impact on functionality or maintainability.

Low

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant