PHOENIX-6267 View Index PK Fixed Width Field Truncation #1021
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
gjacoby126
requested changes
Dec 17, 2020
phoenix-core/src/it/java/org/apache/phoenix/end2end/index/ViewIndexIT.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/it/java/org/apache/phoenix/end2end/index/ViewIndexIT.java
Show resolved
Hide resolved
| + " CONSTRAINT PK PRIMARY KEY (ORGANIZATION_ID, KEY_PREFIX)" | ||
| + " )VERSIONS=1, IMMUTABLE_ROWS=%s, MULTI_TENANT=%s, REPLICATION_SCOPE=1, IMMUTABLE_STORAGE_SCHEME=ONE_CELL_PER_COLUMN, COLUMN_ENCODED_BYTES=0", fullTableName, (isNamespaceMapped? "true" : "false"),(isNamespaceMapped? "true" : "false")); | ||
| String viewDdl = String.format("CREATE VIEW IF NOT EXISTS %s (DATE_TIME1 DATE NOT NULL, TEXT1 VARCHAR, TEXT3 VARCHAR, INT1 BIGINT NOT NULL, DATE_TIME2 DATE, DATE_TIME3 DATE, INT2 BIGINT, INT3 INTEGER, DOUBLE1 DECIMAL(12, 3), " | ||
| + "DOUBLE2 DECIMAL(12, 3), DOUBLE3 DECIMAL(12, 3), TEXT2 VARCHAR, TEXT4 VARCHAR, EMAIL1 VARCHAR, PHONE1 VARCHAR, URL1 VARCHAR " |
There was a problem hiding this comment.
Do we need this many fields to reproduce the issue / prove its fixed? Looks like we never use more than 5 at a time?
There was a problem hiding this comment.
@gjacoby126 Let's keep it so that we can play with more scenarios in the future :)
I had to change the code a bit because unit tests were failing. Now the code looks more like the the datarowkey generation.
phoenix-core/src/it/java/org/apache/phoenix/end2end/index/ViewIndexIT.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/it/java/org/apache/phoenix/end2end/index/ViewIndexIT.java
Outdated
Show resolved
Hide resolved
gokceni
force-pushed
the
PHOENIX-6267
branch
from
9bb4171
to
702a63d
Compare
kadirozde
reviewed
Dec 17, 2020
phoenix-core/src/main/java/org/apache/phoenix/index/IndexMaintainer.java
Show resolved
Hide resolved
kadirozde
reviewed
Dec 17, 2020
phoenix-core/src/main/java/org/apache/phoenix/index/IndexMaintainer.java
Outdated
Show resolved
Hide resolved
gokceni
force-pushed
the
PHOENIX-6267
branch
from
702a63d
to
7a4dc7b
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
gjacoby126
requested changes
Dec 17, 2020
| output.writeByte(SchemaUtil.getSeparatorByte(rowKeyOrderOptimizable, ptr.getLength() == 0, isIndexColumnDesc ? SortOrder.DESC : SortOrder.ASC)); | ||
| byte sepByte = SchemaUtil.getSeparatorByte(rowKeyOrderOptimizable, ptr.getLength() == 0, isIndexColumnDesc ? SortOrder.DESC : SortOrder.ASC); | ||
| output.writeByte(sepByte); | ||
| trailingVariableWitdthColumnNum++; |
There was a problem hiding this comment.
nit: variable name not spelled correctly (Width not Witdth)
phoenix-core/src/main/java/org/apache/phoenix/index/IndexMaintainer.java
Outdated
Show resolved
Hide resolved
gokceni
force-pushed
the
PHOENIX-6267
branch
from
7a4dc7b
to
456d816
Compare
kadirozde
requested changes
Dec 17, 2020
phoenix-core/src/it/java/org/apache/phoenix/end2end/index/ViewIndexIT.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
gokceni
force-pushed
the
PHOENIX-6267
branch
from
456d816
to
a0e7ed1
Compare
|
💔 -1 overall
This message was automatically generated. |
kadirozde
approved these changes
Dec 19, 2020
gjacoby126
requested changes
Dec 21, 2020
| upsertStmt.executeUpdate(); | ||
| conn2.commit(); | ||
|
|
||
| String select = "SELECT INT1 FROM " + childViewName |
There was a problem hiding this comment.
@gokceni @kadirozde Shouldn't this be checking whatever the last column of the view PK is, to see if it's been truncated or not? In some of your test cases calling this method, INT1 is not the end of the view PK.
I'd expect cases where, say, it ends in a text field to have a String whose ending is either SEPARATOR_BYTE or DESC_SEPARATOR_BYTE as appropriate and verify that the String isn't truncated. Or have I misunderstood the purpose of the test?
There was a problem hiding this comment.
Yes, whatever the last PK column is, we need to check its value. I missed that we only check INT1 here. Good catch! The test method can take extra input for the last PK column and expected value for it and verify it.
There was a problem hiding this comment.
Check it out. Most of the time, they were all ending with INT1. There is a couple of double and text too. @gjacoby126
gokceni
force-pushed
the
PHOENIX-6267
branch
from
a0e7ed1
to
bef67c8
Compare
|
💔 -1 overall
This message was automatically generated. |
|
JIRA is resolved, closing PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.