Fixes for Multisearch and Append command

[ahkcs]

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

2. 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

3. 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

[ykmr1224]

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.

[ahkcs]

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

[songkant-aws]

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

[songkant-aws]

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

[ahkcs]

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

[songkant-aws]

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

[ahkcs]

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

[songkant-aws]

Got it. Thanks for the change.

[Swiddis]

issue (non-blocking): Should we think about indices with different timestamp field names?

[ahkcs]

Currently it is set as a limitation: we want to only support @timestamp

[songkant-aws]

There is another concern of using this method to allow type merge. If index A's RelDataType 'INTEGER NOT NULL' is put to unified schema, index B's same name RelDataType 'INTEGER' will be merged silently. Index B's column values could contain NULL values.

The generated code could ignore the null check because the merged unified schema has 'iNTEGER NOT NULL'. It will probably throw NPE when merging index B's NULL values. We could write some query to double check if we can reproduce this scenario.

[songkant-aws]

I tried some queries in my local test. Haven't seen such NPE errors yet. Not sure if there is edge case. But for now we can leave it there until needed fix.

[ahkcs]

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

2. 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

3. 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

[ykmr1224]

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.

[ahkcs]

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

[songkant-aws]

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

[songkant-aws]

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

[ahkcs]

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

[songkant-aws]

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

[ahkcs]

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

[songkant-aws]

Got it. Thanks for the change.

[Swiddis]

issue (non-blocking): Should we think about indices with different timestamp field names?

[ahkcs]

Currently it is set as a limitation: we want to only support @timestamp

[songkant-aws]

There is another concern of using this method to allow type merge. If index A's RelDataType 'INTEGER NOT NULL' is put to unified schema, index B's same name RelDataType 'INTEGER' will be merged silently. Index B's column values could contain NULL values.

The generated code could ignore the null check because the merged unified schema has 'iNTEGER NOT NULL'. It will probably throw NPE when merging index B's NULL values. We could write some query to double check if we can reproduce this scenario.

[songkant-aws]

I tried some queries in my local test. Haven't seen such NPE errors yet. Not sure if there is edge case. But for now we can leave it there until needed fix.

[ahkcs]

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

2. 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

3. 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

[ykmr1224]

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.

[ahkcs]

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

[songkant-aws]

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

[songkant-aws]

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

[ahkcs]

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

[songkant-aws]

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

[ahkcs]

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

[songkant-aws]

Got it. Thanks for the change.

[Swiddis]

issue (non-blocking): Should we think about indices with different timestamp field names?

[ahkcs]

Currently it is set as a limitation: we want to only support @timestamp

[songkant-aws]

There is another concern of using this method to allow type merge. If index A's RelDataType 'INTEGER NOT NULL' is put to unified schema, index B's same name RelDataType 'INTEGER' will be merged silently. Index B's column values could contain NULL values.

The generated code could ignore the null check because the merged unified schema has 'iNTEGER NOT NULL'. It will probably throw NPE when merging index B's NULL values. We could write some query to double check if we can reproduce this scenario.

[songkant-aws]

I tried some queries in my local test. Haven't seen such NPE errors yet. Not sure if there is edge case. But for now we can leave it there until needed fix.

[ahkcs]

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

2. 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

3. 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

[ykmr1224]

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.

[ahkcs]

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

[songkant-aws]

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

[songkant-aws]

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

[ahkcs]

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

[songkant-aws]

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

[ahkcs]

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

[songkant-aws]

Got it. Thanks for the change.

[Swiddis]

issue (non-blocking): Should we think about indices with different timestamp field names?

[ahkcs]

Currently it is set as a limitation: we want to only support @timestamp

[songkant-aws]

There is another concern of using this method to allow type merge. If index A's RelDataType 'INTEGER NOT NULL' is put to unified schema, index B's same name RelDataType 'INTEGER' will be merged silently. Index B's column values could contain NULL values.

The generated code could ignore the null check because the merged unified schema has 'iNTEGER NOT NULL'. It will probably throw NPE when merging index B's NULL values. We could write some query to double check if we can reproduce this scenario.

[songkant-aws]

I tried some queries in my local test. Haven't seen such NPE errors yet. Not sure if there is edge case. But for now we can leave it there until needed fix.

[ahkcs]

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

2. 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

3. 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

[ykmr1224]

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.

[ahkcs]

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

[songkant-aws]

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

[songkant-aws]

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

[ahkcs]

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

[songkant-aws]

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

[ahkcs]

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

[songkant-aws]

Got it. Thanks for the change.

[Swiddis]

issue (non-blocking): Should we think about indices with different timestamp field names?

[ahkcs]

Currently it is set as a limitation: we want to only support @timestamp

[songkant-aws]

There is another concern of using this method to allow type merge. If index A's RelDataType 'INTEGER NOT NULL' is put to unified schema, index B's same name RelDataType 'INTEGER' will be merged silently. Index B's column values could contain NULL values.

The generated code could ignore the null check because the merged unified schema has 'iNTEGER NOT NULL'. It will probably throw NPE when merging index B's NULL values. We could write some query to double check if we can reproduce this scenario.

[songkant-aws]

I tried some queries in my local test. Haven't seen such NPE errors yet. Not sure if there is edge case. But for now we can leave it there until needed fix.

[ahkcs]

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

2. 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

3. 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

[ykmr1224]

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.

[ahkcs]

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

[songkant-aws]

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

[songkant-aws]

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

[ahkcs]

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

[songkant-aws]

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

[ahkcs]

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

[songkant-aws]

Got it. Thanks for the change.

[Swiddis]

issue (non-blocking): Should we think about indices with different timestamp field names?

[ahkcs]

Currently it is set as a limitation: we want to only support @timestamp

[songkant-aws]

There is another concern of using this method to allow type merge. If index A's RelDataType 'INTEGER NOT NULL' is put to unified schema, index B's same name RelDataType 'INTEGER' will be merged silently. Index B's column values could contain NULL values.

The generated code could ignore the null check because the merged unified schema has 'iNTEGER NOT NULL'. It will probably throw NPE when merging index B's NULL values. We could write some query to double check if we can reproduce this scenario.

[songkant-aws]

I tried some queries in my local test. Haven't seen such NPE errors yet. Not sure if there is edge case. But for now we can leave it there until needed fix.

[ahkcs]

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

2. 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

3. 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

[ykmr1224]

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.

[ahkcs]

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

[songkant-aws]

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

[songkant-aws]

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

[ahkcs]

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

[songkant-aws]

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

[ahkcs]

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

[songkant-aws]

Got it. Thanks for the change.

[Swiddis]

issue (non-blocking): Should we think about indices with different timestamp field names?

[ahkcs]

Currently it is set as a limitation: we want to only support @timestamp

[songkant-aws]

There is another concern of using this method to allow type merge. If index A's RelDataType 'INTEGER NOT NULL' is put to unified schema, index B's same name RelDataType 'INTEGER' will be merged silently. Index B's column values could contain NULL values.

The generated code could ignore the null check because the merged unified schema has 'iNTEGER NOT NULL'. It will probably throw NPE when merging index B's NULL values. We could write some query to double check if we can reproduce this scenario.

[songkant-aws]

I tried some queries in my local test. Haven't seen such NPE errors yet. Not sure if there is edge case. But for now we can leave it there until needed fix.

[ahkcs]

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

2. 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

3. 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

[ykmr1224]

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.

[ahkcs]

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

[songkant-aws]

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

[songkant-aws]

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

[ahkcs]

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

[songkant-aws]

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

[ahkcs]

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

[songkant-aws]

Got it. Thanks for the change.

[Swiddis]

issue (non-blocking): Should we think about indices with different timestamp field names?

[ahkcs]

Currently it is set as a limitation: we want to only support @timestamp

[songkant-aws]

There is another concern of using this method to allow type merge. If index A's RelDataType 'INTEGER NOT NULL' is put to unified schema, index B's same name RelDataType 'INTEGER' will be merged silently. Index B's column values could contain NULL values.

The generated code could ignore the null check because the merged unified schema has 'iNTEGER NOT NULL'. It will probably throw NPE when merging index B's NULL values. We could write some query to double check if we can reproduce this scenario.

[songkant-aws]

I tried some queries in my local test. Haven't seen such NPE errors yet. Not sure if there is edge case. But for now we can leave it there until needed fix.