-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[enhancement](inverted index) ensure consistent ordering of properties for inverted index show #51467
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.
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
Conversation
…s for inverted index show
|
run buildall |
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the inverted index implementation to always output its properties in a consistent, alphabetical order and adds tests to verify this behavior.
- Refactored
Index.getPropertiesString()to use a sorted map for deterministic ordering. - Added
IndexPropertiesOrderTestto validate consistent property string generation. - Added
ShowCreateTableStmtTest.testIndexPropertiesOrderto verify ordering inSHOW CREATE TABLE.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| fe/fe-core/src/main/java/org/apache/doris/catalog/Index.java | Switched to TreeMap in getPropertiesString() for sorting. |
| fe/fe-core/src/test/java/org/apache/doris/catalog/IndexPropertiesOrderTest.java | New unit test ensuring index properties string is consistent. |
| fe/fe-core/src/test/java/org/apache/doris/analysis/ShowCreateTableStmtTest.java | New test verifying property order in SHOW CREATE TABLE output. |
Comments suppressed due to low confidence (3)
fe/fe-core/src/test/java/org/apache/doris/analysis/ShowCreateTableStmtTest.java:102
- Using
assertTrue(...contains(...))on a long, concatenated string can make tests brittle. Consider asserting the exact expected substring withassertEqualsor using a regex/matcher to validate the property order more robustly.
Assertions.assertTrue(showSql1.contains("INDEX index_name (`name`) USING INVERTED " +
fe/fe-core/src/test/java/org/apache/doris/catalog/IndexPropertiesOrderTest.java:58
- The test confirms each property is present but does not strictly enforce their ordering. Consider adding assertions that check the index positions (e.g.,
props1.indexOf("lower_case") < props1.indexOf("parser")) to ensure the alphabetical order is correctly applied.
Assertions.assertTrue(props1.contains("lower_case"), "Properties should contain lower_case");
fe/fe-core/src/main/java/org/apache/doris/catalog/Index.java:149
- [nitpick] Consider importing
java.util.TreeMapat the top of the file instead of using the fully qualified name inline for cleaner and more readable code.
return "(" + new PrintableMap(new java.util.TreeMap<>(properties), "=", true, false, ",").toString() + ")";
…s for inverted index show
|
run buildall |
…s for inverted index show
|
run buildall |
TPC-H: Total hot run time: 33658 ms |
TPC-DS: Total hot run time: 185423 ms |
ClickBench: Total hot run time: 29.3 s |
…s for inverted index show
|
run buildall |
TPC-H: Total hot run time: 33843 ms |
TPC-DS: Total hot run time: 186107 ms |
ClickBench: Total hot run time: 29.21 s |
eldenmoon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
…s for inverted index show (#51467) Problem Summary: This PR enhances the inverted index implementation to always output its properties in a consistent, alphabetical order and adds tests to verify this behavior.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
This PR enhances the inverted index implementation to always output its properties in a consistent, alphabetical order and adds tests to verify this behavior.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)