Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-6434 Secondary Indexes on PHOENIX_ROW_TIMESTAMP() - #1194
Conversation
stoty
commented
Apr 12, 2021
💔 -1 overall
This message was automatically generated. |
stoty
commented
Apr 14, 2021
💔 -1 overall
This message was automatically generated. |
stoty
commented
Apr 15, 2021
💔 -1 overall
This message was automatically generated. |
stoty
commented
Apr 16, 2021
💔 -1 overall
This message was automatically generated. |
| String timeZoneID = Calendar.getInstance().getTimeZone().getID(); | ||
| // Write a query to get the val2 = 'bc' with a time range query | ||
| String query = "SELECT val1, val2, PHOENIX_ROW_TIMESTAMP() from " + dataTableName + " WHERE val1 = 'bc' AND " + |
There was a problem hiding this comment.
Mind adding some /*+ NO_INDEX */ cases? (if they break disable them, but would be important to have)
There was a problem hiding this comment.
I attempted to add a case with /*+ NO_INDEX */ but it failed. it seems PHOENIX_ROW_TIMESTAMP() does not work with MultiKeyValueComparisonFilter. Please note this is not related to the no index hint, it is a bug in general for data table with or without an index. I will file a bug on this.
There was a problem hiding this comment.
Yeah... I found in my tests that these fail as well. Maybe comment them with a jira number... Not important, though.
There was a problem hiding this comment.
I noticed that my test for /*+ NO_INDEX / was wrong and that's why it failed. I/+ NO_INDEX */ worked for me.
There was a problem hiding this comment.
Nice!
I definitely found that SELECT COUNT(DISTINCT(phoenix_row_timestamp())) FROM T returns the wrong results. That is due to an incorrect plan being generated, though, not related to this. I filed a separate jira for that.
| @Override | ||
| public Determinism getDeterminism() { | ||
| return Determinism.PER_ROW; | ||
| return Determinism.ALWAYS; |
There was a problem hiding this comment.
Why is this needed? And is it correct? (The row timestamp is per row.)
There was a problem hiding this comment.
There is a check in MetaDataClient#createIndex(), which does not allow non-deterministic expressions in index and considers that anything other than Determinism.ALWAYS is non-deterministic. This does not seem right. SQL Server agrees with me "Deterministic functions always return the same result any time they are called with a specific set of input values and given the same state of the database. Nondeterministic functions may return different results each time they are called with a specific set of input values even if the database state that they access remains the same. For example, the function AVG always returns the same result given the qualifications stated above, but the GETDATE function, which returns the current datetime value, always returns a different result." Maybe I need to change the create index code to allow PER_ROW determinism too. What do you say?
There was a problem hiding this comment.
Hmm... Interesting. If you do a transaction (or query) over 1000 rows you would not expect that PHOENIX_ROW_TIMESTAMP() returning the same one value, so ALWAYS seems wrong.
Doing a git blame it looks like I, myself, added PER_INVOCATION with PHOENIX-1516, prior to that there was only ROW, STATEMENT, and ALWAYS.
I agree with you that indexes should be fine with PER_ROW and ALWAYS determinism.
PER_INVOCATION should be disallowed. Not 100% sure about PER_STATEMENT, seems that would cause problems. It is possible that some function are incorrectly labeled, though, we do not seems to follow that logic correctly.
| if (!mutable) { | ||
| return; | ||
| } | ||
| if (!localIndex && useSnapshot) { |
There was a problem hiding this comment.
Is this an unrelated change?
There was a problem hiding this comment.
No, it is not related and I will remove it
stoty
commented
Apr 21, 2021
💔 -1 overall
This message was automatically generated. |
| if (!encoded) { | ||
| optionBuilder.append(" COLUMN_ENCODED_BYTES=0"); | ||
| indexOptionBuilder.append(" IMMUTABLE_STORAGE_SCHEME=SINGLE_CELL_ARRAY_WITH_OFFSETS, COLUMN_ENCODED_BYTES=2"); | ||
| //indexOptionBuilder.append(" IMMUTABLE_STORAGE_SCHEME=SINGLE_CELL_ARRAY_WITH_OFFSETS, COLUMN_ENCODED_BYTES=2"); |
There was a problem hiding this comment.
On purpose? Do you need this for the NO_INDEX parts of the tests to pass?
There was a problem hiding this comment.
No, it was on purpose. I wanted to make sure that it works without the single cell format and forgot to remove it. I will remove it.
| return null; | ||
| } | ||
| Cell cell = cellList.get(0); | ||
| return new KeyValue(cell.getRowArray(), cell.getRowOffset(), cell.getRowLength(), |
There was a problem hiding this comment.
return org.apache.hadoop.hbase.KeyValueUtil.ensureKeyValue(cell)?
There was a problem hiding this comment.
It is deprecated. Should I use it?
There was a problem hiding this comment.
It's deprecated because we're trying to depeicate KeyValue, I think. Hmmm, perhaps better not to use it.
Do you need a KeyValue as output, or would a Cell work as well?
There was a problem hiding this comment.
Casting Cell as KeyValue does not work in all cases. It triggers an HBase exception.
lhofhansl
commented
Apr 21, 2021
Still +1 I applied the changes locally onto the 5.1 branch, and did a bunch of manual tests. Works as expected. |
stoty
commented
Apr 21, 2021
💔 -1 overall
This message was automatically generated. |
No description provided.