Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-6528 Fix view index read repair for the pks with variable length - #1286
PHOENIX-6528 Fix view index read repair for the pks with variable length#1286gokceni wants to merge 1 commit into
Conversation
gokceni
commented
Aug 11, 2021
stoty
commented
Aug 11, 2021
💔 -1 overall
This message was automatically generated. |
stoty
commented
Aug 11, 2021
💔 -1 overall
This message was automatically generated. |
| String actualExplainPlan = QueryUtil.getExplainPlan(rs1); | ||
| assertTrue(actualExplainPlan.contains("_IDX_" + fullTableName)); | ||
| SingleCellIndexIT.dumpTable("_IDX_" + fullTableName); |
There was a problem hiding this comment.
Please remove dumpTable statements before merging
| // (but we still need to write it if it's DESC to ensure sort order is correct). | ||
| byte sepByte = SchemaUtil.getSeparatorByte(rowKeyOrderOptimizable, ptr.getLength() == 0, dataRowKeySchema.getField(i)); | ||
| if (!dataRowKeySchema.getField(i).getDataType().isFixedWidth() && (((i+1) != dataRowKeySchema.getFieldCount()) || sepByte == QueryConstants.DESC_SEPARATOR_BYTE)) { | ||
| if (!dataRowKeySchema.getField(i).getDataType().isFixedWidth()){ |
There was a problem hiding this comment.
Need to fix comment on line 848 which refers to the logic you're changing
| // Remove trailing nulls | ||
| int index = dataRowKeySchema.getFieldCount() - 1; | ||
| while (index >= 0 && !dataRowKeySchema.getField(index).getDataType().isFixedWidth() && length > minLength && dataRowKey[length-1] == QueryConstants.SEPARATOR_BYTE) { | ||
| while (trailingVariableWidthColumnNum > 0 && dataRowKey[length-1] == QueryConstants.SEPARATOR_BYTE) { |
There was a problem hiding this comment.
What if it's the DESC_SEPARATOR_BYTE? wouldn't we need to continue the loop and decrement trailingVariableWidthColumnNum? The old logic only needed to check the asc SEPARATOR_BYTE but that's because it only wrote a SEPARATOR_BYTE in the asc case, but that's changed now above.
There was a problem hiding this comment.
Alternately, do we need to leave the sepByte == QueryConstants.DESC_SEPARATOR_BYTE part of the check back above on 849?
There was a problem hiding this comment.
@gjacoby126 In line 850, if the field is variable length with this new change we write the desc or asc sep byte always regardless of it is last one or not.
In the previous case, if it is a variable length and last field but not desc, we didn't write it but if it is variable len, last but asc, we didn't.
In line 860, if it is desc_sep_byte, we don't continue the loop in before and now. In the old case, it wrote desc_byte all the time even if it is the last field.
For example: Think about this case: col1 VARCHAR, col2 VARCHAR DESC
In the old version it wrote: sepbyteforasc,sepbytefordesc
In the new version it writes: sepbyteforasc,sepbytefordesc
And line 860 doesn't remove these
For this case: col1 VARCHAR, col2 VARCHAR
In old version: sepbyteforasc
In new version: sepbyteforasc,sepbyteforasc
In line 860,
In old version last one is removed, in new version last 2 is removed.
For this case: col1 VARCHAR DESC, col2 VARCHAR
In old version: sepbytefordesc
In new version: sepbytefordesc,sepbyteforasc
In line 860,
In old version last one is not removed, in new version last 1 is removed and they had sepbytefordesc as the only sep byte.
Am I missing something?
stoty
commented
Aug 17, 2021
💔 -1 overall
This message was automatically generated. |
gokceni
commented
Feb 4, 2022
merged |
No description provided.