Skip to content

Conversation

@gokceni
Copy link
Contributor

@gokceni gokceni commented Jan 5, 2022

No description provided.

@gokceni
Copy link
Contributor Author

gokceni commented Jan 5, 2022

@gjacoby126 @virajjasani

@stoty
Copy link
Contributor

stoty commented Jan 5, 2022

I haven't reviewed the code in detail, but could you add some tests to make sure that VERIFIED status is preserved when doing the transform, especially for transactional tables that do not have the index maintainer coprocessor ?
We've had problems like that in IndexTool and BulkLoad before.

@gokceni
Copy link
Contributor Author

gokceni commented Jan 5, 2022

@stoty will do. Do you have a sample test that I can tweak for TransformTool? If the table is transactional and going trough transform and if an update is made, you want to see that the verified flag is set correctly?
Also could you point me to the fix for IndexTool?

@stoty
Copy link
Contributor

stoty commented Jan 5, 2022

The indextool fix is PHOENIX-6611 .
The fix for setting the verified flag for transactional tables in the normal path was also included in that commit.

I am more worried abot the scenario where whe have a (transactional) table with global indexes, and where the rows are in verified state, losing the VERFIED status after transforming.

IndexToolIT.testSecondaryIndex() checks this here:

The above test case checks for mutable, immutable, and transactional tables, doing the same for Transforms may also be useful.

I can see you are adding the option to check all rows in this patch, but not sure if we test that we don't break the already VERFIED rows' status during transform, where we don't need to touch the index rows.

You may have added check for this for the other cases, I haven't read the patch fully.

testVerifiedForTransactionalTable("TEPHRA");
}

private void testVerifiedForTransactionalTable(String provider) throws Exception{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stoty added this test for TEPHRA. For OMID, we cannot have transform for now because the only type we support is changing column encoding and storage format encoding which is not compatible with OMID. When we have different type of transforms, we can add a test for OMID too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gokceni - I see existing error messages UNSUPPORTED_COLUMN_ENCODING_FOR_TXN_PROVIDER and UNSUPPORTED_STORAGE_FORMAT_FOR_TXN_PROVIDER -- do those get thrown on an ALTER of an OMID table or just a CREATE? If just a CREATE, it should probably do so for ALTER too. (Can be a separate JIRA.)

@stoty
Copy link
Contributor

stoty commented Jan 6, 2022

Thank you @gokceni.

@gjacoby126
Copy link
Contributor

@gokceni - looks like there's a compilation error (I think because of the moving of VERIFIED_BYTES in Istvan's recent patch)

@gokceni
Copy link
Contributor Author

gokceni commented Jan 7, 2022

@gjacoby126 fixed. Thanks

Copy link
Contributor

@gjacoby126 gjacoby126 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 @gokceni (but see a question below)

testVerifiedForTransactionalTable("TEPHRA");
}

private void testVerifiedForTransactionalTable(String provider) throws Exception{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gokceni - I see existing error messages UNSUPPORTED_COLUMN_ENCODING_FOR_TXN_PROVIDER and UNSUPPORTED_STORAGE_FORMAT_FOR_TXN_PROVIDER -- do those get thrown on an ALTER of an OMID table or just a CREATE? If just a CREATE, it should probably do so for ALTER too. (Can be a separate JIRA.)

@gokceni
Copy link
Contributor Author

gokceni commented Feb 4, 2022

merged

@gokceni gokceni closed this Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants