Skip to content

[IOTDB-FIX] Fix LimitNode Serialization and Prevent NullPointerException in Relational Engine#17857

Closed
Muhammad-Ikhwan-Fathulloh wants to merge 1 commit into
apache:masterfrom
Muhammad-Ikhwan-Fathulloh:fix-limitnode-serialization
Closed

[IOTDB-FIX] Fix LimitNode Serialization and Prevent NullPointerException in Relational Engine#17857
Muhammad-Ikhwan-Fathulloh wants to merge 1 commit into
apache:masterfrom
Muhammad-Ikhwan-Fathulloh:fix-limitnode-serialization

Conversation

@Muhammad-Ikhwan-Fathulloh
Copy link
Copy Markdown

This PR fixes an issue in LimitNode where tiesResolvingScheme was not fully integrated into the node lifecycle.

Previously, the field was ignored during serialization and deserialization, meaning its state could be lost when query plans were reconstructed. In addition, the deserialization logic passed null to an Optional<OrderingScheme> field, which could lead to potential NullPointerException issues. The field was also not included in equals() and hashCode(), resulting in incomplete object comparisons.

Changes included

  • Added serialization and deserialization support for tiesResolvingScheme.
  • Replaced null assignments with proper Optional.empty() / Optional.of(...) handling to improve null safety.
  • Updated equals() and hashCode() to include tiesResolvingScheme.
  • Removed the outdated TODO comment since the field is now fully utilized.

Impact

This change helps ensure that:

  • Query plan state is preserved correctly during serialization and deserialization.
  • Potential NullPointerException risks are eliminated.
  • LimitNode comparisons accurately reflect the complete node state.
  • Planner behavior remains consistent and reliable when tiesResolvingScheme is present.

Verification

  • Verified that serialization and deserialization follow the same field order.
  • Compared the implementation pattern with similar relational planner nodes such as SortNode.
  • Confirmed that Optional<OrderingScheme> is handled safely without using null.

…gine

This PR addresses a logical bug in the LimitNode where the tiesResolvingScheme field was being ignored during serialization and deserialization. It also fixes a potential NPE in deserialize().
@HTHou HTHou requested review from JackieTien97 and Wei-hao-Li June 8, 2026 02:19
@JackieTien97
Copy link
Copy Markdown
Contributor

actually tiesResolvingScheme is never used in current implementation, so we didn't serialize it.

@Muhammad-Ikhwan-Fathulloh
Copy link
Copy Markdown
Author

Muhammad-Ikhwan-Fathulloh commented Jun 8, 2026

actually tiesResolvingScheme is never used in current implementation, so we didn't serialize it.

Thank you for the clarification, @JackieTien97. I misunderstood the purpose of tiesResolvingScheme and assumed it was an incomplete implementation that needed to be integrated. I apologize for the confusion and the breaking changes caused by this PR.

Given that this field is intentionally left unused at this stage, I will close this PR to avoid any unnecessary modifications to the serialization logic. Thank you for your time and guidance.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants