-
Notifications
You must be signed in to change notification settings - Fork 1k
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
PHOENIX-7263 Row value constructor split keys not allowed on indexes #1850
base: master
Are you sure you want to change the base?
Conversation
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.
I would prefer that that the test enumerated and checked the created regions explicitly.
assertEquals("chara", rs.getString(1)); | ||
assertEquals(4, rs.getInt(2)); | ||
assertFalse(rs.next()); | ||
conn.createStatement().execute("DROP INDEX " + indexName + " ON " + fullTableName); |
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.
Nit: We already have a stmt object. We could also create the stmt in the try-with-reources block.
+ (uncovered ? "" : " INCLUDE (long_col1, long_col2)") + (localIndex ? "" : | ||
" SPLIT ON (" + indexSplitKeys + ")"); | ||
stmt.execute(ddl); | ||
|
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.
We could check the regions and start keys explicitly.
I think that there is already test code for that and we'd just need to use / copy that.
import java.sql.SQLException; | ||
import java.util.List; | ||
|
||
public class SplitKeyUtil { |
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.
Do we need a new Util class ?
Do we have an existing Util class that would make sense for this ?
The checkstyle errors also look relevant. |
@stoty while preparing the split keys index key constraints like order also should be considered. Will change the patch accordingly and will check whether it can be moved to existing utils. |
No description provided.