fix: allow LIST/VALUES aggregates to accept DATE/TIMESTAMP/IP/BINARY field types by waterWang · Pull Request #5669 · opensearch-project/sql

waterWang · 2026-08-01T21:33:55Z

Description

This PR fixes a type-matching bug in the PPL aggregate function validation that caused LIST and VALUES aggregate functions to reject DATE, TIMESTAMP, IP, and BINARY field types, even though these types were explicitly listed in the allowed signatures.

Root Cause

The typesMatch() method in PPLTypeChecker.java was returning false when one operand was a UDT (via AbstractExprRelDataType) and the other was a plain RelDataType. Database-sourced fields (e.g., TIMESTAMP from a table) arrive as plain RelDataType, but the allowed SCALAR_TYPES list in PPLOperandTypes.java uses UDT variants for temporal/special types (e.g., TIMESTAMP_UDT, DATE_UDT, IP_UDT, BINARY_UDT).

This caused the validation to fail with:

Aggregation function LIST expects field type {[BYTE]|[SHORT]|[INTEGER]|[LONG]|[FLOAT]|[DOUBLE]|[STRING]|[BOOLEAN]|[DATE]|[TIME]|[TIMESTAMP]|[IP]|[BINARY]}, but got [TIMESTAMP]

Fix

Changed typesMatch() to fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain RelDataType, instead of returning false.

Related Issues

Fixesopensearch-project/OpenSearch#22581

…P/BINARY field types The typesMatch() method in PPLTypeChecker was returning false when one operand was a UDT (AbstractExprRelDataType) and the other was a plain RelDataType. This caused LIST and VALUES aggregate functions to reject DATE/TIMESTAMP/IP/BINARY fields from database-sourced tables, even though these types were listed in the allowed signatures. The fix falls back to SqlTypeName comparison when the UDT-vs-plain mismatch occurs, so a TIMESTAMP field from a table matches the TIMESTAMP_UDT in the allowed SCALAR_TYPES list. Fixesopensearch-project/OpenSearch#22581

github-actions · 2026-08-01T21:35:19Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Incomplete Type Match The fix assumes that `SqlTypeName` comparison is sufficient when one operand is a UDT and the other is plain `RelDataType`. However, if both `expected` and `actual` are plain `RelDataType` instances but represent semantically different types that happen to share the same `SqlTypeName`, this could produce false positives. The original code explicitly rejected mixed UDT/plain comparisons, suggesting there may be cases where `SqlTypeName` alone is insufficient to determine type compatibility. Consider whether additional validation is needed to ensure the plain `RelDataType` truly corresponds to the UDT's semantic type. // Fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain// RelDataType. This covers database-sourced fields (e.g. TIMESTAMP) that arrive as plain// RelDataType against allowed signatures using UDT variants (e.g. TIMESTAMP_UDT).returnexpected.getSqlTypeName() == actual.getSqlTypeName();

github-actions · 2026-08-01T21:35:30Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null safety check Add null safety check before calling `getSqlTypeName()` on both `expected` and `actual` parameters. If either parameter is null, the method will throw a NullPointerException when attempting to call `getSqlTypeName()`. core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java [566-569] // Fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain // RelDataType. This covers database-sourced fields (e.g. TIMESTAMP) that arrive as plain // RelDataType against allowed signatures using UDT variants (e.g. TIMESTAMP_UDT). +if (expected == null \|\| actual == null) {+ return false;+} return expected.getSqlTypeName() == actual.getSqlTypeName(); Suggestion importance[1-10]: 5 __ Why: While adding null checks is generally good practice, the suggestion assumes `expected` and `actual` can be null without evidence from the PR context. The method signature and existing code don't indicate null handling is needed, and the removed code didn't check for nulls either. This is a defensive programming suggestion that may or may not be necessary depending on the broader codebase contract.	Low

dai-chen

Please add IT to verify.

waterWang · 2026-08-01T21:33:55Z

Description

This PR fixes a type-matching bug in the PPL aggregate function validation that caused LIST and VALUES aggregate functions to reject DATE, TIMESTAMP, IP, and BINARY field types, even though these types were explicitly listed in the allowed signatures.

Root Cause

The typesMatch() method in PPLTypeChecker.java was returning false when one operand was a UDT (via AbstractExprRelDataType) and the other was a plain RelDataType. Database-sourced fields (e.g., TIMESTAMP from a table) arrive as plain RelDataType, but the allowed SCALAR_TYPES list in PPLOperandTypes.java uses UDT variants for temporal/special types (e.g., TIMESTAMP_UDT, DATE_UDT, IP_UDT, BINARY_UDT).

This caused the validation to fail with:

Aggregation function LIST expects field type {[BYTE]|[SHORT]|[INTEGER]|[LONG]|[FLOAT]|[DOUBLE]|[STRING]|[BOOLEAN]|[DATE]|[TIME]|[TIMESTAMP]|[IP]|[BINARY]}, but got [TIMESTAMP]

Fix

Changed typesMatch() to fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain RelDataType, instead of returning false.

Related Issues

Fixesopensearch-project/OpenSearch#22581

…P/BINARY field types The typesMatch() method in PPLTypeChecker was returning false when one operand was a UDT (AbstractExprRelDataType) and the other was a plain RelDataType. This caused LIST and VALUES aggregate functions to reject DATE/TIMESTAMP/IP/BINARY fields from database-sourced tables, even though these types were listed in the allowed signatures. The fix falls back to SqlTypeName comparison when the UDT-vs-plain mismatch occurs, so a TIMESTAMP field from a table matches the TIMESTAMP_UDT in the allowed SCALAR_TYPES list. Fixesopensearch-project/OpenSearch#22581

github-actions · 2026-08-01T21:35:19Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Incomplete Type Match The fix assumes that `SqlTypeName` comparison is sufficient when one operand is a UDT and the other is plain `RelDataType`. However, if both `expected` and `actual` are plain `RelDataType` instances but represent semantically different types that happen to share the same `SqlTypeName`, this could produce false positives. The original code explicitly rejected mixed UDT/plain comparisons, suggesting there may be cases where `SqlTypeName` alone is insufficient to determine type compatibility. Consider whether additional validation is needed to ensure the plain `RelDataType` truly corresponds to the UDT's semantic type. // Fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain// RelDataType. This covers database-sourced fields (e.g. TIMESTAMP) that arrive as plain// RelDataType against allowed signatures using UDT variants (e.g. TIMESTAMP_UDT).returnexpected.getSqlTypeName() == actual.getSqlTypeName();

github-actions · 2026-08-01T21:35:30Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null safety check Add null safety check before calling `getSqlTypeName()` on both `expected` and `actual` parameters. If either parameter is null, the method will throw a NullPointerException when attempting to call `getSqlTypeName()`. core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java [566-569] // Fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain // RelDataType. This covers database-sourced fields (e.g. TIMESTAMP) that arrive as plain // RelDataType against allowed signatures using UDT variants (e.g. TIMESTAMP_UDT). +if (expected == null \|\| actual == null) {+ return false;+} return expected.getSqlTypeName() == actual.getSqlTypeName(); Suggestion importance[1-10]: 5 __ Why: While adding null checks is generally good practice, the suggestion assumes `expected` and `actual` can be null without evidence from the PR context. The method signature and existing code don't indicate null handling is needed, and the removed code didn't check for nulls either. This is a defensive programming suggestion that may or may not be necessary depending on the broader codebase contract.	Low

dai-chen

Please add IT to verify.

waterWang · 2026-08-01T21:33:55Z

Description

This PR fixes a type-matching bug in the PPL aggregate function validation that caused LIST and VALUES aggregate functions to reject DATE, TIMESTAMP, IP, and BINARY field types, even though these types were explicitly listed in the allowed signatures.

Root Cause

The typesMatch() method in PPLTypeChecker.java was returning false when one operand was a UDT (via AbstractExprRelDataType) and the other was a plain RelDataType. Database-sourced fields (e.g., TIMESTAMP from a table) arrive as plain RelDataType, but the allowed SCALAR_TYPES list in PPLOperandTypes.java uses UDT variants for temporal/special types (e.g., TIMESTAMP_UDT, DATE_UDT, IP_UDT, BINARY_UDT).

This caused the validation to fail with:

Aggregation function LIST expects field type {[BYTE]|[SHORT]|[INTEGER]|[LONG]|[FLOAT]|[DOUBLE]|[STRING]|[BOOLEAN]|[DATE]|[TIME]|[TIMESTAMP]|[IP]|[BINARY]}, but got [TIMESTAMP]

Fix

Changed typesMatch() to fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain RelDataType, instead of returning false.

Related Issues

Fixesopensearch-project/OpenSearch#22581

…P/BINARY field types The typesMatch() method in PPLTypeChecker was returning false when one operand was a UDT (AbstractExprRelDataType) and the other was a plain RelDataType. This caused LIST and VALUES aggregate functions to reject DATE/TIMESTAMP/IP/BINARY fields from database-sourced tables, even though these types were listed in the allowed signatures. The fix falls back to SqlTypeName comparison when the UDT-vs-plain mismatch occurs, so a TIMESTAMP field from a table matches the TIMESTAMP_UDT in the allowed SCALAR_TYPES list. Fixesopensearch-project/OpenSearch#22581

github-actions · 2026-08-01T21:35:19Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Incomplete Type Match The fix assumes that `SqlTypeName` comparison is sufficient when one operand is a UDT and the other is plain `RelDataType`. However, if both `expected` and `actual` are plain `RelDataType` instances but represent semantically different types that happen to share the same `SqlTypeName`, this could produce false positives. The original code explicitly rejected mixed UDT/plain comparisons, suggesting there may be cases where `SqlTypeName` alone is insufficient to determine type compatibility. Consider whether additional validation is needed to ensure the plain `RelDataType` truly corresponds to the UDT's semantic type. // Fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain// RelDataType. This covers database-sourced fields (e.g. TIMESTAMP) that arrive as plain// RelDataType against allowed signatures using UDT variants (e.g. TIMESTAMP_UDT).returnexpected.getSqlTypeName() == actual.getSqlTypeName();

github-actions · 2026-08-01T21:35:30Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null safety check Add null safety check before calling `getSqlTypeName()` on both `expected` and `actual` parameters. If either parameter is null, the method will throw a NullPointerException when attempting to call `getSqlTypeName()`. core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java [566-569] // Fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain // RelDataType. This covers database-sourced fields (e.g. TIMESTAMP) that arrive as plain // RelDataType against allowed signatures using UDT variants (e.g. TIMESTAMP_UDT). +if (expected == null \|\| actual == null) {+ return false;+} return expected.getSqlTypeName() == actual.getSqlTypeName(); Suggestion importance[1-10]: 5 __ Why: While adding null checks is generally good practice, the suggestion assumes `expected` and `actual` can be null without evidence from the PR context. The method signature and existing code don't indicate null handling is needed, and the removed code didn't check for nulls either. This is a defensive programming suggestion that may or may not be necessary depending on the broader codebase contract.	Low

dai-chen

Please add IT to verify.

waterWang · 2026-08-01T21:33:55Z

Description

This PR fixes a type-matching bug in the PPL aggregate function validation that caused LIST and VALUES aggregate functions to reject DATE, TIMESTAMP, IP, and BINARY field types, even though these types were explicitly listed in the allowed signatures.

Root Cause

The typesMatch() method in PPLTypeChecker.java was returning false when one operand was a UDT (via AbstractExprRelDataType) and the other was a plain RelDataType. Database-sourced fields (e.g., TIMESTAMP from a table) arrive as plain RelDataType, but the allowed SCALAR_TYPES list in PPLOperandTypes.java uses UDT variants for temporal/special types (e.g., TIMESTAMP_UDT, DATE_UDT, IP_UDT, BINARY_UDT).

This caused the validation to fail with:

Aggregation function LIST expects field type {[BYTE]|[SHORT]|[INTEGER]|[LONG]|[FLOAT]|[DOUBLE]|[STRING]|[BOOLEAN]|[DATE]|[TIME]|[TIMESTAMP]|[IP]|[BINARY]}, but got [TIMESTAMP]

Fix

Changed typesMatch() to fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain RelDataType, instead of returning false.

Related Issues

Fixesopensearch-project/OpenSearch#22581

…P/BINARY field types The typesMatch() method in PPLTypeChecker was returning false when one operand was a UDT (AbstractExprRelDataType) and the other was a plain RelDataType. This caused LIST and VALUES aggregate functions to reject DATE/TIMESTAMP/IP/BINARY fields from database-sourced tables, even though these types were listed in the allowed signatures. The fix falls back to SqlTypeName comparison when the UDT-vs-plain mismatch occurs, so a TIMESTAMP field from a table matches the TIMESTAMP_UDT in the allowed SCALAR_TYPES list. Fixesopensearch-project/OpenSearch#22581

github-actions · 2026-08-01T21:35:19Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Incomplete Type Match The fix assumes that `SqlTypeName` comparison is sufficient when one operand is a UDT and the other is plain `RelDataType`. However, if both `expected` and `actual` are plain `RelDataType` instances but represent semantically different types that happen to share the same `SqlTypeName`, this could produce false positives. The original code explicitly rejected mixed UDT/plain comparisons, suggesting there may be cases where `SqlTypeName` alone is insufficient to determine type compatibility. Consider whether additional validation is needed to ensure the plain `RelDataType` truly corresponds to the UDT's semantic type. // Fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain// RelDataType. This covers database-sourced fields (e.g. TIMESTAMP) that arrive as plain// RelDataType against allowed signatures using UDT variants (e.g. TIMESTAMP_UDT).returnexpected.getSqlTypeName() == actual.getSqlTypeName();

github-actions · 2026-08-01T21:35:30Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null safety check Add null safety check before calling `getSqlTypeName()` on both `expected` and `actual` parameters. If either parameter is null, the method will throw a NullPointerException when attempting to call `getSqlTypeName()`. core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java [566-569] // Fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain // RelDataType. This covers database-sourced fields (e.g. TIMESTAMP) that arrive as plain // RelDataType against allowed signatures using UDT variants (e.g. TIMESTAMP_UDT). +if (expected == null \|\| actual == null) {+ return false;+} return expected.getSqlTypeName() == actual.getSqlTypeName(); Suggestion importance[1-10]: 5 __ Why: While adding null checks is generally good practice, the suggestion assumes `expected` and `actual` can be null without evidence from the PR context. The method signature and existing code don't indicate null handling is needed, and the removed code didn't check for nulls either. This is a defensive programming suggestion that may or may not be necessary depending on the broader codebase contract.	Low

dai-chen

Please add IT to verify.

waterWang · 2026-08-01T21:33:55Z

Description

This PR fixes a type-matching bug in the PPL aggregate function validation that caused LIST and VALUES aggregate functions to reject DATE, TIMESTAMP, IP, and BINARY field types, even though these types were explicitly listed in the allowed signatures.

Root Cause

The typesMatch() method in PPLTypeChecker.java was returning false when one operand was a UDT (via AbstractExprRelDataType) and the other was a plain RelDataType. Database-sourced fields (e.g., TIMESTAMP from a table) arrive as plain RelDataType, but the allowed SCALAR_TYPES list in PPLOperandTypes.java uses UDT variants for temporal/special types (e.g., TIMESTAMP_UDT, DATE_UDT, IP_UDT, BINARY_UDT).

This caused the validation to fail with:

Aggregation function LIST expects field type {[BYTE]|[SHORT]|[INTEGER]|[LONG]|[FLOAT]|[DOUBLE]|[STRING]|[BOOLEAN]|[DATE]|[TIME]|[TIMESTAMP]|[IP]|[BINARY]}, but got [TIMESTAMP]

Fix

Changed typesMatch() to fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain RelDataType, instead of returning false.

Related Issues

Fixesopensearch-project/OpenSearch#22581

…P/BINARY field types The typesMatch() method in PPLTypeChecker was returning false when one operand was a UDT (AbstractExprRelDataType) and the other was a plain RelDataType. This caused LIST and VALUES aggregate functions to reject DATE/TIMESTAMP/IP/BINARY fields from database-sourced tables, even though these types were listed in the allowed signatures. The fix falls back to SqlTypeName comparison when the UDT-vs-plain mismatch occurs, so a TIMESTAMP field from a table matches the TIMESTAMP_UDT in the allowed SCALAR_TYPES list. Fixesopensearch-project/OpenSearch#22581

github-actions · 2026-08-01T21:35:19Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Incomplete Type Match The fix assumes that `SqlTypeName` comparison is sufficient when one operand is a UDT and the other is plain `RelDataType`. However, if both `expected` and `actual` are plain `RelDataType` instances but represent semantically different types that happen to share the same `SqlTypeName`, this could produce false positives. The original code explicitly rejected mixed UDT/plain comparisons, suggesting there may be cases where `SqlTypeName` alone is insufficient to determine type compatibility. Consider whether additional validation is needed to ensure the plain `RelDataType` truly corresponds to the UDT's semantic type. // Fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain// RelDataType. This covers database-sourced fields (e.g. TIMESTAMP) that arrive as plain// RelDataType against allowed signatures using UDT variants (e.g. TIMESTAMP_UDT).returnexpected.getSqlTypeName() == actual.getSqlTypeName();

github-actions · 2026-08-01T21:35:30Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null safety check Add null safety check before calling `getSqlTypeName()` on both `expected` and `actual` parameters. If either parameter is null, the method will throw a NullPointerException when attempting to call `getSqlTypeName()`. core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java [566-569] // Fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain // RelDataType. This covers database-sourced fields (e.g. TIMESTAMP) that arrive as plain // RelDataType against allowed signatures using UDT variants (e.g. TIMESTAMP_UDT). +if (expected == null \|\| actual == null) {+ return false;+} return expected.getSqlTypeName() == actual.getSqlTypeName(); Suggestion importance[1-10]: 5 __ Why: While adding null checks is generally good practice, the suggestion assumes `expected` and `actual` can be null without evidence from the PR context. The method signature and existing code don't indicate null handling is needed, and the removed code didn't check for nulls either. This is a defensive programming suggestion that may or may not be necessary depending on the broader codebase contract.	Low

dai-chen

Please add IT to verify.

waterWang · 2026-08-01T21:33:55Z

Description

This PR fixes a type-matching bug in the PPL aggregate function validation that caused LIST and VALUES aggregate functions to reject DATE, TIMESTAMP, IP, and BINARY field types, even though these types were explicitly listed in the allowed signatures.

Root Cause

The typesMatch() method in PPLTypeChecker.java was returning false when one operand was a UDT (via AbstractExprRelDataType) and the other was a plain RelDataType. Database-sourced fields (e.g., TIMESTAMP from a table) arrive as plain RelDataType, but the allowed SCALAR_TYPES list in PPLOperandTypes.java uses UDT variants for temporal/special types (e.g., TIMESTAMP_UDT, DATE_UDT, IP_UDT, BINARY_UDT).

This caused the validation to fail with:

Aggregation function LIST expects field type {[BYTE]|[SHORT]|[INTEGER]|[LONG]|[FLOAT]|[DOUBLE]|[STRING]|[BOOLEAN]|[DATE]|[TIME]|[TIMESTAMP]|[IP]|[BINARY]}, but got [TIMESTAMP]

Fix

Changed typesMatch() to fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain RelDataType, instead of returning false.

Related Issues

Fixesopensearch-project/OpenSearch#22581

…P/BINARY field types The typesMatch() method in PPLTypeChecker was returning false when one operand was a UDT (AbstractExprRelDataType) and the other was a plain RelDataType. This caused LIST and VALUES aggregate functions to reject DATE/TIMESTAMP/IP/BINARY fields from database-sourced tables, even though these types were listed in the allowed signatures. The fix falls back to SqlTypeName comparison when the UDT-vs-plain mismatch occurs, so a TIMESTAMP field from a table matches the TIMESTAMP_UDT in the allowed SCALAR_TYPES list. Fixesopensearch-project/OpenSearch#22581

github-actions · 2026-08-01T21:35:19Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Incomplete Type Match The fix assumes that `SqlTypeName` comparison is sufficient when one operand is a UDT and the other is plain `RelDataType`. However, if both `expected` and `actual` are plain `RelDataType` instances but represent semantically different types that happen to share the same `SqlTypeName`, this could produce false positives. The original code explicitly rejected mixed UDT/plain comparisons, suggesting there may be cases where `SqlTypeName` alone is insufficient to determine type compatibility. Consider whether additional validation is needed to ensure the plain `RelDataType` truly corresponds to the UDT's semantic type. // Fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain// RelDataType. This covers database-sourced fields (e.g. TIMESTAMP) that arrive as plain// RelDataType against allowed signatures using UDT variants (e.g. TIMESTAMP_UDT).returnexpected.getSqlTypeName() == actual.getSqlTypeName();

github-actions · 2026-08-01T21:35:30Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null safety check Add null safety check before calling `getSqlTypeName()` on both `expected` and `actual` parameters. If either parameter is null, the method will throw a NullPointerException when attempting to call `getSqlTypeName()`. core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java [566-569] // Fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain // RelDataType. This covers database-sourced fields (e.g. TIMESTAMP) that arrive as plain // RelDataType against allowed signatures using UDT variants (e.g. TIMESTAMP_UDT). +if (expected == null \|\| actual == null) {+ return false;+} return expected.getSqlTypeName() == actual.getSqlTypeName(); Suggestion importance[1-10]: 5 __ Why: While adding null checks is generally good practice, the suggestion assumes `expected` and `actual` can be null without evidence from the PR context. The method signature and existing code don't indicate null handling is needed, and the removed code didn't check for nulls either. This is a defensive programming suggestion that may or may not be necessary depending on the broader codebase contract.	Low

dai-chen

Please add IT to verify.

waterWang · 2026-08-01T21:33:55Z

Description

This PR fixes a type-matching bug in the PPL aggregate function validation that caused LIST and VALUES aggregate functions to reject DATE, TIMESTAMP, IP, and BINARY field types, even though these types were explicitly listed in the allowed signatures.

Root Cause

The typesMatch() method in PPLTypeChecker.java was returning false when one operand was a UDT (via AbstractExprRelDataType) and the other was a plain RelDataType. Database-sourced fields (e.g., TIMESTAMP from a table) arrive as plain RelDataType, but the allowed SCALAR_TYPES list in PPLOperandTypes.java uses UDT variants for temporal/special types (e.g., TIMESTAMP_UDT, DATE_UDT, IP_UDT, BINARY_UDT).

This caused the validation to fail with:

Aggregation function LIST expects field type {[BYTE]|[SHORT]|[INTEGER]|[LONG]|[FLOAT]|[DOUBLE]|[STRING]|[BOOLEAN]|[DATE]|[TIME]|[TIMESTAMP]|[IP]|[BINARY]}, but got [TIMESTAMP]

Fix

Changed typesMatch() to fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain RelDataType, instead of returning false.

Related Issues

Fixesopensearch-project/OpenSearch#22581

…P/BINARY field types The typesMatch() method in PPLTypeChecker was returning false when one operand was a UDT (AbstractExprRelDataType) and the other was a plain RelDataType. This caused LIST and VALUES aggregate functions to reject DATE/TIMESTAMP/IP/BINARY fields from database-sourced tables, even though these types were listed in the allowed signatures. The fix falls back to SqlTypeName comparison when the UDT-vs-plain mismatch occurs, so a TIMESTAMP field from a table matches the TIMESTAMP_UDT in the allowed SCALAR_TYPES list. Fixesopensearch-project/OpenSearch#22581

github-actions · 2026-08-01T21:35:19Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Incomplete Type Match The fix assumes that `SqlTypeName` comparison is sufficient when one operand is a UDT and the other is plain `RelDataType`. However, if both `expected` and `actual` are plain `RelDataType` instances but represent semantically different types that happen to share the same `SqlTypeName`, this could produce false positives. The original code explicitly rejected mixed UDT/plain comparisons, suggesting there may be cases where `SqlTypeName` alone is insufficient to determine type compatibility. Consider whether additional validation is needed to ensure the plain `RelDataType` truly corresponds to the UDT's semantic type. // Fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain// RelDataType. This covers database-sourced fields (e.g. TIMESTAMP) that arrive as plain// RelDataType against allowed signatures using UDT variants (e.g. TIMESTAMP_UDT).returnexpected.getSqlTypeName() == actual.getSqlTypeName();

github-actions · 2026-08-01T21:35:30Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null safety check Add null safety check before calling `getSqlTypeName()` on both `expected` and `actual` parameters. If either parameter is null, the method will throw a NullPointerException when attempting to call `getSqlTypeName()`. core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java [566-569] // Fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain // RelDataType. This covers database-sourced fields (e.g. TIMESTAMP) that arrive as plain // RelDataType against allowed signatures using UDT variants (e.g. TIMESTAMP_UDT). +if (expected == null \|\| actual == null) {+ return false;+} return expected.getSqlTypeName() == actual.getSqlTypeName(); Suggestion importance[1-10]: 5 __ Why: While adding null checks is generally good practice, the suggestion assumes `expected` and `actual` can be null without evidence from the PR context. The method signature and existing code don't indicate null handling is needed, and the removed code didn't check for nulls either. This is a defensive programming suggestion that may or may not be necessary depending on the broader codebase contract.	Low

dai-chen

Please add IT to verify.

waterWang · 2026-08-01T21:33:55Z

Description

This PR fixes a type-matching bug in the PPL aggregate function validation that caused LIST and VALUES aggregate functions to reject DATE, TIMESTAMP, IP, and BINARY field types, even though these types were explicitly listed in the allowed signatures.

Root Cause

The typesMatch() method in PPLTypeChecker.java was returning false when one operand was a UDT (via AbstractExprRelDataType) and the other was a plain RelDataType. Database-sourced fields (e.g., TIMESTAMP from a table) arrive as plain RelDataType, but the allowed SCALAR_TYPES list in PPLOperandTypes.java uses UDT variants for temporal/special types (e.g., TIMESTAMP_UDT, DATE_UDT, IP_UDT, BINARY_UDT).

This caused the validation to fail with:

Aggregation function LIST expects field type {[BYTE]|[SHORT]|[INTEGER]|[LONG]|[FLOAT]|[DOUBLE]|[STRING]|[BOOLEAN]|[DATE]|[TIME]|[TIMESTAMP]|[IP]|[BINARY]}, but got [TIMESTAMP]

Fix

Changed typesMatch() to fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain RelDataType, instead of returning false.

Related Issues

Fixesopensearch-project/OpenSearch#22581

…P/BINARY field types The typesMatch() method in PPLTypeChecker was returning false when one operand was a UDT (AbstractExprRelDataType) and the other was a plain RelDataType. This caused LIST and VALUES aggregate functions to reject DATE/TIMESTAMP/IP/BINARY fields from database-sourced tables, even though these types were listed in the allowed signatures. The fix falls back to SqlTypeName comparison when the UDT-vs-plain mismatch occurs, so a TIMESTAMP field from a table matches the TIMESTAMP_UDT in the allowed SCALAR_TYPES list. Fixesopensearch-project/OpenSearch#22581

github-actions · 2026-08-01T21:35:19Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Incomplete Type Match The fix assumes that `SqlTypeName` comparison is sufficient when one operand is a UDT and the other is plain `RelDataType`. However, if both `expected` and `actual` are plain `RelDataType` instances but represent semantically different types that happen to share the same `SqlTypeName`, this could produce false positives. The original code explicitly rejected mixed UDT/plain comparisons, suggesting there may be cases where `SqlTypeName` alone is insufficient to determine type compatibility. Consider whether additional validation is needed to ensure the plain `RelDataType` truly corresponds to the UDT's semantic type. // Fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain// RelDataType. This covers database-sourced fields (e.g. TIMESTAMP) that arrive as plain// RelDataType against allowed signatures using UDT variants (e.g. TIMESTAMP_UDT).returnexpected.getSqlTypeName() == actual.getSqlTypeName();

github-actions · 2026-08-01T21:35:30Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null safety check Add null safety check before calling `getSqlTypeName()` on both `expected` and `actual` parameters. If either parameter is null, the method will throw a NullPointerException when attempting to call `getSqlTypeName()`. core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java [566-569] // Fall back to SqlTypeName comparison when one operand is a UDT and the other is a plain // RelDataType. This covers database-sourced fields (e.g. TIMESTAMP) that arrive as plain // RelDataType against allowed signatures using UDT variants (e.g. TIMESTAMP_UDT). +if (expected == null \|\| actual == null) {+ return false;+} return expected.getSqlTypeName() == actual.getSqlTypeName(); Suggestion importance[1-10]: 5 __ Why: While adding null checks is generally good practice, the suggestion assumes `expected` and `actual` can be null without evidence from the PR context. The method signature and existing code don't indicate null handling is needed, and the removed code didn't check for nulls either. This is a defensive programming suggestion that may or may not be necessary depending on the broader codebase contract.	Low

dai-chen

Please add IT to verify.

waterWang requested review from LantaoJin, RyanL1997, Swiddis, acarbonetto, ahkcs, anirudha, dai-chen, joshuali925, mengweieric, noCharger, penghuo, ps48, qianheng-aws, songkant-aws, vamsimanohar, ykmr1224 and yuancu as code owners August 1, 2026 21:33

dai-chen reviewed Aug 18, 2026
View reviewed changes

waterWang requested review from LantaoJin, RyanL1997, Swiddis, acarbonetto, ahkcs, anirudha, dai-chen, joshuali925, mengweieric, noCharger, penghuo, ps48, qianheng-aws, songkant-aws, vamsimanohar, ykmr1224 and yuancu as code owners August 1, 2026 21:33

dai-chen reviewed Aug 18, 2026
View reviewed changes

waterWang requested review from LantaoJin, RyanL1997, Swiddis, acarbonetto, ahkcs, anirudha, dai-chen, joshuali925, mengweieric, noCharger, penghuo, ps48, qianheng-aws, songkant-aws, vamsimanohar, ykmr1224 and yuancu as code owners August 1, 2026 21:33

dai-chen reviewed Aug 18, 2026
View reviewed changes

waterWang requested review from LantaoJin, RyanL1997, Swiddis, acarbonetto, ahkcs, anirudha, dai-chen, joshuali925, mengweieric, noCharger, penghuo, ps48, qianheng-aws, songkant-aws, vamsimanohar, ykmr1224 and yuancu as code owners August 1, 2026 21:33

dai-chen reviewed Aug 18, 2026
View reviewed changes

waterWang requested review from LantaoJin, RyanL1997, Swiddis, acarbonetto, ahkcs, anirudha, dai-chen, joshuali925, mengweieric, noCharger, penghuo, ps48, qianheng-aws, songkant-aws, vamsimanohar, ykmr1224 and yuancu as code owners August 1, 2026 21:33

dai-chen reviewed Aug 18, 2026
View reviewed changes

waterWang requested review from LantaoJin, RyanL1997, Swiddis, acarbonetto, ahkcs, anirudha, dai-chen, joshuali925, mengweieric, noCharger, penghuo, ps48, qianheng-aws, songkant-aws, vamsimanohar, ykmr1224 and yuancu as code owners August 1, 2026 21:33

dai-chen reviewed Aug 18, 2026
View reviewed changes

waterWang requested review from LantaoJin, RyanL1997, Swiddis, acarbonetto, ahkcs, anirudha, dai-chen, joshuali925, mengweieric, noCharger, penghuo, ps48, qianheng-aws, songkant-aws, vamsimanohar, ykmr1224 and yuancu as code owners August 1, 2026 21:33

dai-chen reviewed Aug 18, 2026
View reviewed changes

waterWang requested review from LantaoJin, RyanL1997, Swiddis, acarbonetto, ahkcs, anirudha, dai-chen, joshuali925, mengweieric, noCharger, penghuo, ps48, qianheng-aws, songkant-aws, vamsimanohar, ykmr1224 and yuancu as code owners August 1, 2026 21:33

dai-chen reviewed Aug 18, 2026
View reviewed changes

Uh oh!

Conversation

waterWang commented Aug 1, 2026

Description

Root Cause

Fix

Related Issues

Uh oh!

github-actionsBot commented Aug 1, 2026

PR Reviewer Guide 🔍

Uh oh!

github-actionsBot commented Aug 1, 2026

PR Code Suggestions ✨

Uh oh!

dai-chen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

waterWang commented Aug 1, 2026

Description

Root Cause

Fix

Related Issues

Uh oh!

github-actionsBot commented Aug 1, 2026

PR Reviewer Guide 🔍

Uh oh!

github-actionsBot commented Aug 1, 2026

PR Code Suggestions ✨

Uh oh!

dai-chen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

waterWang commented Aug 1, 2026

Description

Root Cause

Fix

Related Issues

Uh oh!

github-actionsBot commented Aug 1, 2026

PR Reviewer Guide 🔍

Uh oh!

github-actionsBot commented Aug 1, 2026

PR Code Suggestions ✨

Uh oh!

dai-chen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

waterWang commented Aug 1, 2026

Description

Root Cause