Skip to content

Fixes for Multisearch and Append command #4512

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Fixes for Multisearch and Append command #4512

wants to merge 6 commits into from

Conversation

ahkcs
Copy link
Contributor

@ahkcs ahkcs commented Oct 10, 2025 •
edited
Loading

Description

  1. Type Conflict Handling
  • Changed behavior: Type conflicts now throw IllegalArgumentException instead of auto-renaming fields
  • Modified: SchemaUnifier.java - Removed automatic field renaming logic (e.g., age → age0)
  • Updated Tests:
    • CalciteMultisearchCommandIT.testMultisearchWithDirectTypeConflict - Now expects exception
    • CalcitePPLAppendCommandIT.testAppendWithConflictTypeColumn - Now expects exception
  • Documentation: Updated multisearch.rst and append.rst to reflect new behavior and add Limitations section
  1. Timestamp Interleaving
  • Modified: CalciteRelNodeVisitor.findTimestampField() - Now only detects @timestamp field for timestamp interleaving
  • Other timestamp fields (_time, timestamp, time) are no longer used for interleaving
  1. Documentation Fixes
  • multisearch.rst:
    • Added Limitations section
    • Removed redundant Example 4 (Handling Empty Results)
    • Removed Example 6 (type-conflict resolution)
  • append.rst:
    • Added Limitations section
    • Removed Example 3 (type conflict example no longer valid)

Related PRs

#4332 #4123

@ahkcs ahkcs changed the title Fixes for Multisearch and Append command Fixes for Multisearch and Append command Oct 10, 2025
ahkcs added 4 commits October 10, 2025 14:12
@ahkcs
Fix for Multisearch and Append command
63cc39f 
Signed-off-by: Kai Huang <ahkcs@amazon.com>
@ahkcs
fix order
f914e02 
Signed-off-by: Kai Huang <ahkcs@amazon.com>
@ahkcs
fix tests
5887750 
Signed-off-by: Kai Huang <ahkcs@amazon.com>
@ahkcs
fix test
b9332ed 
Signed-off-by: Kai Huang <ahkcs@amazon.com>
@ahkcs ahkcs marked this pull request as ready for review October 10, 2025 21:28
ykmr1224
ykmr1224 reviewed Oct 10, 2025
View reviewed changes
core/src/main/java/org/opensearch/sql/calcite/SchemaUnifier.java Outdated
Comment on lines 92 to 97
if (!typesForName.contains(fieldType)) {
// New field or same name with different type - add to schema
schema.add(new SchemaField(fieldName, fieldType));
typesForName.add(fieldType);
}
// If we've seen this exact (name, type) combination, skip it
Copy link
Collaborator

@ykmr1224 ykmr1224 Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should rather raise error from here. (Then we don't need to check again)
Later we would implement a logic to generalize types for the same field name here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion! I think it makes sense, moved the location to raise error

@ahkcs
remove error location
cbc80e8 
Signed-off-by: Kai Huang <ahkcs@amazon.com>
@ahkcs ahkcs requested a review from ykmr1224 October 10, 2025 22:19
songkant-aws
songkant-aws reviewed Oct 11, 2025
View reviewed changes
core/src/main/java/org/opensearch/sql/calcite/SchemaUnifier.java

if (!typesForName.contains(fieldType)) {
// New field or same name with different type - add to schema
RelDataType existingType = seenFields.get(fieldName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We recently find another issue of type conflicts here. RelDataType evaluates the hash equality by its digested string as well. For example, "INTEGER" is not equal to "INTEGER NOT NULL". A quick fix would be aligning the same SqlType to be nullable. Ideally it won't affect the data type resolution while execution. cc @xinyual

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can consider to allow same SqlTypeName but with different nullability to be merged here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion! I have updated the implementation to allow same SqlTypeName but with different nullability to be merged

docs/user/ppl/cmd/append.rst
Limitations
===========

* **Schema Compatibility**: When fields with the same name exist between the main search and sub-search but have incompatible types, the query will fail with an error. To avoid type conflicts, ensure that fields with the same name have the same data type, or use different field names (e.g., by renaming with ``eval`` or using ``fields`` to select non-conflicting columns).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understand the intention here. Strong schema engine like SQL restricts the type to be the same. Some weak schema engine resolves types at runtime and doesn't care the data type. I think it's not easy to make it compatible.

Not sure what's better user experience and customer expectation here. Does user accept this behavior or expect to union anyway? cc @LantaoJin

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are planning to enable permissive mode in the future: #4349 to support schema merging with type conflicts, in order to avoid breaking changes in the future, we are marking this as a limitation now instead of using a workaround. cc @penghuo

ykmr1224
ykmr1224 previously approved these changes Oct 15, 2025
View reviewed changes
@ahkcs
Allow same SqlTypeName but with different nullability to be merged
98e40bc 
Signed-off-by: Kai Huang <ahkcs@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ykmr1224 ykmr1224 ykmr1224 left review comments

@ps48 ps48 Awaiting requested review from ps48 ps48 is a code owner

@kavithacm kavithacm Awaiting requested review from kavithacm kavithacm is a code owner

@derek-ho derek-ho Awaiting requested review from derek-ho derek-ho is a code owner

@joshuali925 joshuali925 Awaiting requested review from joshuali925 joshuali925 is a code owner

@dai-chen dai-chen Awaiting requested review from dai-chen dai-chen is a code owner

@YANG-DB YANG-DB Awaiting requested review from YANG-DB YANG-DB is a code owner

@mengweieric mengweieric Awaiting requested review from mengweieric mengweieric is a code owner

@vamsimanohar vamsimanohar Awaiting requested review from vamsimanohar vamsimanohar is a code owner

@Swiddis Swiddis Awaiting requested review from Swiddis Swiddis is a code owner

@penghuo penghuo Awaiting requested review from penghuo penghuo is a code owner

@seankao-az seankao-az Awaiting requested review from seankao-az seankao-az is a code owner

@MaxKsyunz MaxKsyunz Awaiting requested review from MaxKsyunz MaxKsyunz is a code owner

@Yury-Fridlyand Yury-Fridlyand Awaiting requested review from Yury-Fridlyand Yury-Fridlyand is a code owner

@anirudha anirudha Awaiting requested review from anirudha anirudha is a code owner

@forestmvey forestmvey Awaiting requested review from forestmvey forestmvey is a code owner

@acarbonetto acarbonetto Awaiting requested review from acarbonetto acarbonetto is a code owner

@GumpacG GumpacG Awaiting requested review from GumpacG GumpacG is a code owner

@LantaoJin LantaoJin Awaiting requested review from LantaoJin LantaoJin is a code owner

@noCharger noCharger Awaiting requested review from noCharger noCharger is a code owner

@qianheng-aws qianheng-aws Awaiting requested review from qianheng-aws qianheng-aws is a code owner

@yuancu yuancu Awaiting requested review from yuancu yuancu is a code owner

@RyanL1997 RyanL1997 Awaiting requested review from RyanL1997 RyanL1997 is a code owner

+1 more reviewer

@songkant-aws songkant-aws songkant-aws left review comments

Reviewers whose approvals may not affect merge requirements

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ahkcs @ykmr1224 @songkant-aws