Fix multisearch UDT type loss through UNION (#5145, #5146, #5147)#5154
Fix multisearch UDT type loss through UNION (#5145, #5146, #5147)#5154ahkcs wants to merge 4 commits intoopensearch-project:mainfrom
Conversation
Signed-off-by: Kai Huang <ahkcs@amazon.com>
ahkcs
requested review from
GumpacG,
LantaoJin,
MaxKsyunz,
RyanL1997,
Swiddis,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
noCharger,
penghuo,
ps48,
qianheng-aws,
seankao-az,
vamsimanohar,
ykmr1224 and
yuancu
as code owners
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a leastRestrictive override to preserve OpenSearch UDT wrappers with proper nullability handling, updates PPL/multisearch integration tests (adds reproduction tests for issues Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java`:
- Around line 381-409: Update the JavaDoc for the public override
leastRestrictive in OpenSearchTypeFactory to include `@param` types (describe that
it is the list of input RelDataType), `@return` (the least restrictive
RelDataType, preserving UDT wrapper when applicable) and `@throws` (document any
runtime exceptions or pass-through from super.leastRestrictive if relevant);
then add unit tests covering: 1) UDT preservation—when all inputs are
AbstractExprRelDataType with the same getUdt() the result must keep that UDT
wrapper (use AbstractExprRelDataType test doubles), 2) mixed nullability—when
any input is nullable and the first is non-nullable the returned type becomes
nullable via createWithNullability(this, true), and 3) mixed UDT/non-UDT
inputs—when inputs include non-UDT types the method must fall back to
super.leastRestrictive; reference the method name leastRestrictive, the class
OpenSearchTypeFactory, and AbstractExprRelDataType in tests and assertions.
In
`@integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteMultisearchCommandIT.java`:
- Around line 371-542: The three tests testMultisearchWithSpanExpression,
testMultisearchBinTimestamp, and testMultisearchBinAndStats rely on implicit
result ordering and must be made deterministic: modify the query passed to
executeQuery to include an explicit sort (e.g., add "| sort `@timestamp` asc" or
"| sort <field> asc/desc" after the final pipeline stage) so results are
consistently ordered, then update the expected verifyDataRows rows(...)
sequences to match the new sorted order (or alternatively change verifyDataRows
calls to use an order-agnostic assertion helper); update only those tests and
keep the rest of the assertions intact.
| /** | ||
| * Preserves OpenSearch UDT types through set operations (UNION, INTERSECT, EXCEPT). When all | ||
| * input types share the same UDT (e.g., all are EXPR_TIMESTAMP), the result retains the UDT | ||
| * wrapper instead of being downgraded to the underlying SQL type (e.g., VARCHAR). This is | ||
| * critical for operations like multisearch that use UNION ALL, where downstream operators (bin, | ||
| * span) rely on the UDT type to determine how to process the field. | ||
| */ | ||
| @Override | ||
| public @Nullable RelDataType leastRestrictive(List<RelDataType> types) { | ||
| if (types.size() > 1) { | ||
| RelDataType first = types.get(0); | ||
| if (first instanceof AbstractExprRelDataType<?> firstUdt) { | ||
| boolean allSameUdt = | ||
| types.stream() | ||
| .allMatch( | ||
| t -> | ||
| t instanceof AbstractExprRelDataType<?> udt | ||
| && udt.getUdt() == firstUdt.getUdt()); | ||
| if (allSameUdt) { | ||
| boolean anyNullable = types.stream().anyMatch(RelDataType::isNullable); | ||
| if (anyNullable && !first.isNullable()) { | ||
| return firstUdt.createWithNullability(this, true); | ||
| } | ||
| return first; | ||
| } | ||
| } | ||
| } | ||
| return super.leastRestrictive(types); | ||
| } |
There was a problem hiding this comment.
Add required JavaDoc tags and unit tests for the new leastRestrictive override.
This new public method is missing @param/@return/@throws tags, and the commit doesn’t add unit tests for the new behavior (UDT preservation, mixed nullability, mixed UDT/non‑UDT inputs). Please add unit tests and update the JavaDoc accordingly.
✍️ Add the required JavaDoc tags
- /**
- * Preserves OpenSearch UDT types through set operations (UNION, INTERSECT, EXCEPT). When all
- * input types share the same UDT (e.g., all are EXPR_TIMESTAMP), the result retains the UDT
- * wrapper instead of being downgraded to the underlying SQL type (e.g., VARCHAR). This is
- * critical for operations like multisearch that use UNION ALL, where downstream operators (bin,
- * span) rely on the UDT type to determine how to process the field.
- */
+ /**
+ * Preserves OpenSearch UDT types through set operations (UNION, INTERSECT, EXCEPT). When all
+ * input types share the same UDT (e.g., all are EXPR_TIMESTAMP), the result retains the UDT
+ * wrapper instead of being downgraded to the underlying SQL type (e.g., VARCHAR). This is
+ * critical for operations like multisearch that use UNION ALL, where downstream operators (bin,
+ * span) rely on the UDT type to determine how to process the field.
+ *
+ * `@param` types candidate types to reconcile
+ * `@return` least restrictive type that preserves UDT when possible
+ * `@throws` NullPointerException if {`@code` types} is null
+ */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java`
around lines 381 - 409, Update the JavaDoc for the public override
leastRestrictive in OpenSearchTypeFactory to include `@param` types (describe that
it is the list of input RelDataType), `@return` (the least restrictive
RelDataType, preserving UDT wrapper when applicable) and `@throws` (document any
runtime exceptions or pass-through from super.leastRestrictive if relevant);
then add unit tests covering: 1) UDT preservation—when all inputs are
AbstractExprRelDataType with the same getUdt() the result must keep that UDT
wrapper (use AbstractExprRelDataType test doubles), 2) mixed nullability—when
any input is nullable and the first is non-nullable the returned type becomes
nullable via createWithNullability(this, true), and 3) mixed UDT/non-UDT
inputs—when inputs include non-UDT types the method must fall back to
super.leastRestrictive; reference the method name leastRestrictive, the class
OpenSearchTypeFactory, and AbstractExprRelDataType in tests and assertions.
| /** Reproduce #5146: span expression used after multisearch should work. */ | ||
| @Test | ||
| public void testMultisearchWithSpanExpression() throws IOException { | ||
| JSONObject result = | ||
| executeQuery( | ||
| "| multisearch [search source=opensearch-sql_test_index_time_data | where category =" | ||
| + " \\\"A\\\"] [search source=opensearch-sql_test_index_time_data2 | where category" | ||
| + " = \\\"E\\\"] | stats avg(value) by span(@timestamp, 5m)"); | ||
|
|
||
| verifySchema( | ||
| result, | ||
| schema("avg(value)", null, "double"), | ||
| schema("span(@timestamp,5m)", null, "timestamp")); | ||
|
|
||
| // Each data point falls in its own 5-min bucket (all >5min apart), so avg = single value | ||
| // Category A: 26 rows from time_test_data, Category E: 10 rows from time_test_data2 | ||
| verifyDataRows( | ||
| result, | ||
| // Category A (26 rows) | ||
| rows(8945.0, "2025-07-28 00:15:00"), | ||
| rows(6834.0, "2025-07-28 03:55:00"), | ||
| rows(6589.0, "2025-07-28 07:50:00"), | ||
| rows(9367.0, "2025-07-28 11:05:00"), | ||
| rows(9245.0, "2025-07-28 15:15:00"), | ||
| rows(8917.0, "2025-07-28 19:20:00"), | ||
| rows(8384.0, "2025-07-28 23:30:00"), | ||
| rows(8798.0, "2025-07-29 03:35:00"), | ||
| rows(9306.0, "2025-07-29 07:45:00"), | ||
| rows(8873.0, "2025-07-29 11:50:00"), | ||
| rows(8542.0, "2025-07-29 15:00:00"), | ||
| rows(9321.0, "2025-07-29 19:05:00"), | ||
| rows(8917.0, "2025-07-29 23:10:00"), | ||
| rows(8756.0, "2025-07-30 03:20:00"), | ||
| rows(9234.0, "2025-07-30 07:25:00"), | ||
| rows(8679.0, "2025-07-30 11:35:00"), | ||
| rows(8765.0, "2025-07-30 15:40:00"), | ||
| rows(9187.0, "2025-07-30 19:50:00"), | ||
| rows(8862.0, "2025-07-30 23:55:00"), | ||
| rows(8537.0, "2025-07-31 03:00:00"), | ||
| rows(9318.0, "2025-07-31 07:10:00"), | ||
| rows(8914.0, "2025-07-31 11:15:00"), | ||
| rows(8753.0, "2025-07-31 15:25:00"), | ||
| rows(9231.0, "2025-07-31 19:30:00"), | ||
| rows(8676.0, "2025-07-31 23:40:00"), | ||
| rows(8762.0, "2025-08-01 03:45:00"), | ||
| // Category E (10 rows) | ||
| rows(2001.0, "2025-08-01 04:00:00"), | ||
| rows(2003.0, "2025-08-01 01:00:00"), | ||
| rows(2005.0, "2025-07-31 20:45:00"), | ||
| rows(2007.0, "2025-07-31 16:00:00"), | ||
| rows(2009.0, "2025-07-31 12:30:00"), | ||
| rows(2011.0, "2025-07-31 08:00:00"), | ||
| rows(2013.0, "2025-07-31 04:30:00"), | ||
| rows(2015.0, "2025-07-31 01:00:00"), | ||
| rows(2017.0, "2025-07-30 21:30:00"), | ||
| rows(2019.0, "2025-07-30 18:00:00")); | ||
| } | ||
|
|
||
| /** Reproduce #5147: bin command after multisearch should produce non-null @timestamp. */ | ||
| @Test | ||
| public void testMultisearchBinTimestamp() throws IOException { | ||
| JSONObject result = | ||
| executeQuery( | ||
| "| multisearch [search source=opensearch-sql_test_index_time_data | where category =" | ||
| + " \\\"A\\\"] [search source=opensearch-sql_test_index_time_data2 | where category" | ||
| + " = \\\"E\\\"] | fields @timestamp, category, value | bin @timestamp span=5m"); | ||
|
|
||
| verifySchema( | ||
| result, | ||
| schema("category", null, "string"), | ||
| schema("value", null, "int"), | ||
| schema("@timestamp", null, "timestamp")); | ||
|
|
||
| // bin floors @timestamp to 5-min boundaries; projectPlusOverriding moves @timestamp to end | ||
| // Category A: 26 rows from time_test_data, Category E: 10 rows from time_test_data2 | ||
| verifyDataRows( | ||
| result, | ||
| // Category A (26 rows) | ||
| rows("A", 8945, "2025-07-28 00:15:00"), | ||
| rows("A", 6834, "2025-07-28 03:55:00"), | ||
| rows("A", 6589, "2025-07-28 07:50:00"), | ||
| rows("A", 9367, "2025-07-28 11:05:00"), | ||
| rows("A", 9245, "2025-07-28 15:15:00"), | ||
| rows("A", 8917, "2025-07-28 19:20:00"), | ||
| rows("A", 8384, "2025-07-28 23:30:00"), | ||
| rows("A", 8798, "2025-07-29 03:35:00"), | ||
| rows("A", 9306, "2025-07-29 07:45:00"), | ||
| rows("A", 8873, "2025-07-29 11:50:00"), | ||
| rows("A", 8542, "2025-07-29 15:00:00"), | ||
| rows("A", 9321, "2025-07-29 19:05:00"), | ||
| rows("A", 8917, "2025-07-29 23:10:00"), | ||
| rows("A", 8756, "2025-07-30 03:20:00"), | ||
| rows("A", 9234, "2025-07-30 07:25:00"), | ||
| rows("A", 8679, "2025-07-30 11:35:00"), | ||
| rows("A", 8765, "2025-07-30 15:40:00"), | ||
| rows("A", 9187, "2025-07-30 19:50:00"), | ||
| rows("A", 8862, "2025-07-30 23:55:00"), | ||
| rows("A", 8537, "2025-07-31 03:00:00"), | ||
| rows("A", 9318, "2025-07-31 07:10:00"), | ||
| rows("A", 8914, "2025-07-31 11:15:00"), | ||
| rows("A", 8753, "2025-07-31 15:25:00"), | ||
| rows("A", 9231, "2025-07-31 19:30:00"), | ||
| rows("A", 8676, "2025-07-31 23:40:00"), | ||
| rows("A", 8762, "2025-08-01 03:45:00"), | ||
| // Category E (10 rows) | ||
| rows("E", 2001, "2025-08-01 04:00:00"), | ||
| rows("E", 2003, "2025-08-01 01:00:00"), | ||
| rows("E", 2005, "2025-07-31 20:45:00"), | ||
| rows("E", 2007, "2025-07-31 16:00:00"), | ||
| rows("E", 2009, "2025-07-31 12:30:00"), | ||
| rows("E", 2011, "2025-07-31 08:00:00"), | ||
| rows("E", 2013, "2025-07-31 04:30:00"), | ||
| rows("E", 2015, "2025-07-31 01:00:00"), | ||
| rows("E", 2017, "2025-07-30 21:30:00"), | ||
| rows("E", 2019, "2025-07-30 18:00:00")); | ||
| } | ||
|
|
||
| /** Reproduce #5147 full pattern: bin + stats after multisearch. */ | ||
| @Test | ||
| public void testMultisearchBinAndStats() throws IOException { | ||
| JSONObject result = | ||
| executeQuery( | ||
| "| multisearch [search source=opensearch-sql_test_index_time_data | where category =" | ||
| + " \\\"A\\\"] [search source=opensearch-sql_test_index_time_data2 | where category" | ||
| + " = \\\"E\\\"] | bin @timestamp span=5m | stats avg(value) by @timestamp"); | ||
|
|
||
| verifySchema( | ||
| result, schema("avg(value)", null, "double"), schema("@timestamp", null, "timestamp")); | ||
|
|
||
| // Each data point falls in its own 5-min bucket (all >5min apart), so avg = single value | ||
| // Category A: 26 rows from time_test_data, Category E: 10 rows from time_test_data2 | ||
| verifyDataRows( | ||
| result, | ||
| // Category A (26 rows) | ||
| rows(8945.0, "2025-07-28 00:15:00"), | ||
| rows(6834.0, "2025-07-28 03:55:00"), | ||
| rows(6589.0, "2025-07-28 07:50:00"), | ||
| rows(9367.0, "2025-07-28 11:05:00"), | ||
| rows(9245.0, "2025-07-28 15:15:00"), | ||
| rows(8917.0, "2025-07-28 19:20:00"), | ||
| rows(8384.0, "2025-07-28 23:30:00"), | ||
| rows(8798.0, "2025-07-29 03:35:00"), | ||
| rows(9306.0, "2025-07-29 07:45:00"), | ||
| rows(8873.0, "2025-07-29 11:50:00"), | ||
| rows(8542.0, "2025-07-29 15:00:00"), | ||
| rows(9321.0, "2025-07-29 19:05:00"), | ||
| rows(8917.0, "2025-07-29 23:10:00"), | ||
| rows(8756.0, "2025-07-30 03:20:00"), | ||
| rows(9234.0, "2025-07-30 07:25:00"), | ||
| rows(8679.0, "2025-07-30 11:35:00"), | ||
| rows(8765.0, "2025-07-30 15:40:00"), | ||
| rows(9187.0, "2025-07-30 19:50:00"), | ||
| rows(8862.0, "2025-07-30 23:55:00"), | ||
| rows(8537.0, "2025-07-31 03:00:00"), | ||
| rows(9318.0, "2025-07-31 07:10:00"), | ||
| rows(8914.0, "2025-07-31 11:15:00"), | ||
| rows(8753.0, "2025-07-31 15:25:00"), | ||
| rows(9231.0, "2025-07-31 19:30:00"), | ||
| rows(8676.0, "2025-07-31 23:40:00"), | ||
| rows(8762.0, "2025-08-01 03:45:00"), | ||
| // Category E (10 rows) | ||
| rows(2001.0, "2025-08-01 04:00:00"), | ||
| rows(2003.0, "2025-08-01 01:00:00"), | ||
| rows(2005.0, "2025-07-31 20:45:00"), | ||
| rows(2007.0, "2025-07-31 16:00:00"), | ||
| rows(2009.0, "2025-07-31 12:30:00"), | ||
| rows(2011.0, "2025-07-31 08:00:00"), | ||
| rows(2013.0, "2025-07-31 04:30:00"), | ||
| rows(2015.0, "2025-07-31 01:00:00"), | ||
| rows(2017.0, "2025-07-30 21:30:00"), | ||
| rows(2019.0, "2025-07-30 18:00:00")); | ||
| } |
There was a problem hiding this comment.
Add deterministic ordering for the new multisearch reproduction tests.
These tests assert long ordered row lists, but the queries don’t include sort. Result order can vary, which risks flaky tests and breaks test‑independence requirements. Add explicit sorting (and update expected rows accordingly) or use order‑agnostic assertions.
🔧 Example (apply similarly to the other new tests)
- + " = \"E\"] | stats avg(value) by span(`@timestamp`, 5m)");
+ + " = \"E\"] | stats avg(value) by span(`@timestamp`, 5m) | sort span(`@timestamp`, 5m)");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteMultisearchCommandIT.java`
around lines 371 - 542, The three tests testMultisearchWithSpanExpression,
testMultisearchBinTimestamp, and testMultisearchBinAndStats rely on implicit
result ordering and must be made deterministic: modify the query passed to
executeQuery to include an explicit sort (e.g., add "| sort `@timestamp` asc" or
"| sort <field> asc/desc" after the final pipeline stage) so results are
consistently ordered, then update the expected verifyDataRows rows(...)
sequences to match the new sorted order (or alternatively change verifyDataRows
calls to use an order-agnostic assertion helper); update only those tests and
keep the rest of the assertions intact.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java (2)
111-131: Consider adding boundary condition tests for empty list and single UDT.The delegation tests cover single plain type and two plain types, but the boundary cases for empty input and single UDT are not explicitly tested.
♻️ Suggested additional boundary tests
`@Test` public void testLeastRestrictiveDelegatesToSuperForEmptyList() { RelDataType result = TYPE_FACTORY.leastRestrictive(List.of()); // Empty list delegates to super.leastRestrictive which returns null assertNull(result); } `@Test` public void testLeastRestrictiveDelegatesToSuperForSingleUdt() { RelDataType single = TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP); RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(single)); // Single type (even UDT) delegates to super due to size check assertNotNull(result); assertEquals(SqlTypeName.VARCHAR, result.getSqlTypeName()); }As per coding guidelines: "Include boundary condition tests (min/max values, empty inputs) for all new functions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java` around lines 111 - 131, Add two boundary unit tests in OpenSearchTypeFactoryTest to cover empty input and single UDT cases for TYPE_FACTORY.leastRestrictive: add a test that calls TYPE_FACTORY.leastRestrictive(List.of()) and asserts the result is null (delegates to super for empty list), and add a test that creates a single UDT (e.g., TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP)), calls leastRestrictive with that single UDT and asserts the returned type is not null and has the expected SqlTypeName (e.g., VARCHAR) to ensure single-UDT delegation behavior is validated.
85-109: Strengthen assertions in fallback tests to avoid silent passes.The conditional assertions on lines 93-95 and 106-108 will silently pass if
resultisnull, potentially masking regressions. Consider using explicit assertions for the expected behavior.♻️ Proposed fix to strengthen assertions
`@Test` public void testLeastRestrictiveFallsBackForMixedUdtAndNonUdt() { RelDataType udt = TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP); RelDataType plain = TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR); RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(udt, plain)); - // Falls back to super.leastRestrictive which may return a plain type or null - if (result != null) { - assertEquals(SqlTypeName.VARCHAR, result.getSqlTypeName()); - } + // Falls back to super.leastRestrictive — EXPR_TIMESTAMP has VARCHAR underlying type + assertNotNull(result, "Expected non-null result for compatible types"); + assertEquals(SqlTypeName.VARCHAR, result.getSqlTypeName()); } `@Test` public void testLeastRestrictiveFallsBackForDifferentUdts() { RelDataType timestamp = TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP); RelDataType date = TYPE_FACTORY.createUDT(ExprUDT.EXPR_DATE); RelDataType result = TYPE_FACTORY.leastRestrictive(List.of(timestamp, date)); - // Different UDTs — falls back to super.leastRestrictive - if (result != null) { - assertEquals(SqlTypeName.VARCHAR, result.getSqlTypeName()); - } + // Different UDTs — falls back to super.leastRestrictive (both have VARCHAR underlying) + assertNotNull(result, "Expected non-null result for VARCHAR-based UDTs"); + assertEquals(SqlTypeName.VARCHAR, result.getSqlTypeName()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java` around lines 85 - 109, Replace the conditional "if (result != null) { ... }" checks in testLeastRestrictiveFallsBackForMixedUdtAndNonUdt and testLeastRestrictiveFallsBackForDifferentUdts with explicit assertions: assertNotNull(result) immediately after calling TYPE_FACTORY.leastRestrictive(...) and then assertEquals(SqlTypeName.VARCHAR, result.getSqlTypeName()); this ensures the RelDataType result is non-null and its SqlTypeName is verified rather than silently passing when result is null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@core/src/test/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactoryTest.java`:
- Around line 111-131: Add two boundary unit tests in OpenSearchTypeFactoryTest
to cover empty input and single UDT cases for TYPE_FACTORY.leastRestrictive: add
a test that calls TYPE_FACTORY.leastRestrictive(List.of()) and asserts the
result is null (delegates to super for empty list), and add a test that creates
a single UDT (e.g., TYPE_FACTORY.createUDT(ExprUDT.EXPR_TIMESTAMP)), calls
leastRestrictive with that single UDT and asserts the returned type is not null
and has the expected SqlTypeName (e.g., VARCHAR) to ensure single-UDT delegation
behavior is validated.
- Around line 85-109: Replace the conditional "if (result != null) { ... }"
checks in testLeastRestrictiveFallsBackForMixedUdtAndNonUdt and
testLeastRestrictiveFallsBackForDifferentUdts with explicit assertions:
assertNotNull(result) immediately after calling
TYPE_FACTORY.leastRestrictive(...) and then assertEquals(SqlTypeName.VARCHAR,
result.getSqlTypeName()); this ensures the RelDataType result is non-null and
its SqlTypeName is verified rather than silently passing when result is null.
|
This PR is stalled because it has been open for 2 weeks with no activity. |
Swiddis
left a comment
Swiddis
left a comment
There was a problem hiding this comment.
How are we confident that this is always correct outside of the multisearch special case?
My understanding is leastRestrictive is assumed to convert types to the plain SQL types, so keeping around UDTs in some circumstances might quietly break other code's assumptions. But maybe we only hit this branch with multiple types if we're in an expression that needs it, or maybe we don't actually have such an assumption from any caller code.
| if (allSameUdt) { | ||
| boolean anyNullable = types.stream().anyMatch(RelDataType::isNullable); | ||
| if (anyNullable && !first.isNullable()) { | ||
| return firstUdt.createWithNullability(this, true); |
There was a problem hiding this comment.
nit: Could do this in a single pass with a plain for loop instead of splitting the match checks and null checks.
@Override
public @Nullable RelDataType leastRestrictive(List<RelDataType> types) {
if (types.size() > 1) {
RelDataType first = types.get(0);
if (first instanceof AbstractExprRelDataType<?> firstUdt) {
boolean anyNullable = false;
for (RelDataType t : types) {
if (t instanceof AbstractExprRelDataType<?> udt
&& udt.getUdt() == firstUdt.getUdt()) {
anyNullable |= t.isNullable();
} else {
return super.leastRestrictive(types); // If we hit a non-match, fallback
}
}
if (anyNullable && !first.isNullable()) {
return firstUdt.createWithNullability(this, true);
}
return first;
}
}
return super.leastRestrictive(types);
}
| rows("E", 2013, "2025-07-31 04:30:00"), | ||
| rows("E", 2015, "2025-07-31 01:00:00"), | ||
| rows("E", 2017, "2025-07-30 21:30:00"), | ||
| rows("E", 2019, "2025-07-30 18:00:00")); |
There was a problem hiding this comment.
these test assertions are pretty long, is that the minimal reproducing example? would 1-day intervals be clearer?
The leastRestrictive override only activates when all input types are the same |
PR Reviewer Guide 🔍(Review updated until commit 6a30e67)Here are some key observations to aid the review process:
|
Refactor leastRestrictive to use a single for-loop instead of two stream passes. Simplify multisearch integration tests by using 1-day intervals and count() aggregation to reduce verbose 36-row assertions to 5 rows. Signed-off-by: Kai Huang <ahkcs@amazon.com>
PR Code Suggestions ✨Latest suggestions up to 6a30e67
Previous suggestionsSuggestions up to commit 533fbdf
|
ahkcs
force-pushed
the
fix/multisearch-issues-5145-5146-5147
branch
from
533fbdf to
6a30e67
Compare
|
Persistent review updated to latest commit 6a30e67 |
2 participants
Summary
leastRestrictive()inOpenSearchTypeFactoryto preserve UDT types (e.g.,ExprTimestampType) through set operations (UNION ALL) used by multisearchleastRestrictive()downgrades UDT-wrapped types to their underlying SQL type (e.g.,VARCHAR), breaking downstream operators that rely on type checksIssues Resolved
Resolves #5145, resolves #5146, resolves #5147
See investigation details: #5153
Root Cause
OpenSearchTypeFactory(extendsJavaTypeFactoryImpl) did not overrideleastRestrictive(). When Calcite builds UNION ALL for multisearch, it callsleastRestrictive()on column types from both branches. Both branches haveExprTimestampType(a UDT backed bySqlTypeName.VARCHAR), but the default implementation creates a plainBasicSqlType(VARCHAR), losing the UDT wrapper.This caused:
SpanFunction.SpanImplementorchecksfieldType instanceof ExprSqlType→ fails for plain VARCHAR →"Unsupported expr type: STRING"binused aftermultisearchhas no effect #5147:BinnableField.isTimeBasedType()can't recognize plain VARCHAR as time-based → bin produces null timestampsFix
When all input types share the same UDT (e.g., all are
EXPR_TIMESTAMP), the result retains the UDT wrapper. Nullability is handled correctly — if any input is nullable, the result is nullable. Otherwise, delegates to the default Calcite implementation.Test Plan
testMultisearchWithoutFurtherProcessing— basic multisearch returns all 51 rowstestMultisearchWithSpanExpression—stats avg(value) by span(@timestamp, 5m)after multisearch (36 rows verified)testMultisearchBinTimestamp—bin @timestamp span=5mproduces non-null timestamps (36 rows verified)testMultisearchBinAndStats—bin + statsproduces correct time buckets (36 rows verified)testMultisearchWithTimestampInterleavingschema updated from"string"to"timestamp"(correct behavior)