PHOENIX-5018 Index mutations created by UPSERT SELECT will have wrong… #434
Conversation
phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexBuildTimestampIT.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexBuildTimestampIT.java
Show resolved
Hide resolved
phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexBuildTimestampIT.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/compile/ServerBuildIndexCompiler.java
Outdated
Show resolved
Hide resolved
...nix-core/src/main/java/org/apache/phoenix/mapreduce/index/PhoenixServerBuildIndexMapper.java
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/util/PhoenixConfigurationUtil.java
Show resolved
Hide resolved
|
|
||
| ResultSet rs = conn.createStatement().executeQuery(selectSql); | ||
| assertTrue (rs.next()); | ||
| assertTrue(rs.unwrap(PhoenixResultSet.class).getCurrentRow().getValue(0).getTimestamp() >= clock1.initialTime()); |
There was a problem hiding this comment.
@kadirozde - maybe I'm misunderstanding the test, but shouldn't this be checking against an upper bound to make sure that the index was populated with the initial timestamp of 'abc' (and in the second check 'bcd')?
There was a problem hiding this comment.
Thx for noticing it. I will correct it.
phoenix-core/src/main/java/org/apache/phoenix/compile/ServerBuildIndexCompiler.java
Outdated
Show resolved
Hide resolved
| final Job job = Job.getInstance(configuration, jobName); | ||
| job.setJarByClass(IndexTool.class); | ||
| job.setMapOutputKeyClass(ImmutableBytesWritable.class); | ||
| FileOutputFormat.setOutputPath(job, outputPath); |
There was a problem hiding this comment.
Can dispense with the output path, I think
There was a problem hiding this comment.
When I remove the code for setting the output path, there is always one other test failing. There are total of 120 tests. I tried restructuring the IndexTool code so that the output path is set only when bulk loading is enabled but did not help. So, I will leave the code as it is.
phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexBuildTimestampIT.java
Show resolved
Hide resolved
| @@ -211,23 +211,14 @@ public Void run() throws Exception { | |||
|
|
|||
| // we should be able to read the data from another index as well to which we have not given any access to | |||
| // this user | |||
| verifyAllowed(createIndex(indexName2, phoenixTableName), unprivilegedUser); | |||
There was a problem hiding this comment.
@kadirozde - I'm curious what the purpose of these changes are. Have DDL permissions changed? If not, and your changes somehow break the existing test, is there a way to modify the test logic to keep the same test coverage?
There was a problem hiding this comment.
Currently global indexes are built by using UPSERT SELECT, i..e, by reading mutations from the data table and writing the corresponding mutations to the index table. The user does not need to have the write privilege on the data table in order to build an index table. However, in the new approach, an index table is built by replaying the existing mutations back on the data table again (without actually applying them to the data table) for the purpose of reconstructing the index mutations. Such replay attempts are allowed only when the user has the write privilege. Therefore, I had to delete the scenarios that are not applicable anymore.
|
|
||
| String dataTableFullName = SchemaUtil.getTableName( | ||
| tableRef.getTable().getSchemaName().getString(), | ||
| tableRef.getTable().getTableName().getString()); |
There was a problem hiding this comment.
@kadirozde - curious why the refresh is needed here? The tableRef only seems to be used to get the base table name to give to the ServerBuildIndexDDLCompiler
There was a problem hiding this comment.
I needed this for the initial version of the compiler where the index table was searched in the index list of the PTable for the data table. MetadataClient constructs the PTable object for the data table before the index table is created and thus the object does have a reference to the newly created index. The complier has changed and this search was eliminated. So there is no need to refresh the PTable object anymore. I will remove the refresh code.
There was a problem hiding this comment.
Thanks for explaining, @kadirozde . Once the refresh code gets removed, I think this is ready to go so I'll +1 and commit.
There was a problem hiding this comment.
@gjacoby126, I have removed the refresh code. Thank you for discovering this issue, helping me understand the code, and reviewing the patch!
… timestamps