Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHOENIX-7074 DROP CDC Implementation #1713

Merged
merged 9 commits into from
Dec 4, 2023

Conversation

TheNamesRai
Copy link

@TheNamesRai TheNamesRai commented Oct 17, 2023

This PR builds on top of the shallow grammar support added for DROP CDC grammar as part of #1694. This change makes the required changes to DROP CDC object and the Uncovered Index associated with it.

Jira: PHOENIX-7074

Copy link
Contributor

@haridsv haridsv left a comment

Choose a reason for hiding this comment

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

Looks good overall. There is one requirement that came up in a recent discussion that involves protecting the CDC index from getting dropped by the user via DROP INDEX. I wonder if we can include that change as part of the scope of this PR.


String indexName = CDCUtil.getCDCIndexName(statement.getCdcObjName().getName());
// Dropping the uncovered index associated with the CDC Table
return dropTable(schemaName, indexName, parentTableName, PTableType.INDEX, statement.ifExists(), false, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

In create, we are catching SQLExceptions on index creation and translating them on to CDC so that user wouldn't be exposed to the index name that is internal to the CDC implementation. We should probably do something similar here.

Copy link
Author

Choose a reason for hiding this comment

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

Updated: Handled the SQLExceptions from dropping the Index and created a CDC Wrapper Exception on it.

I think we need not worry about dropping the CDC index directly as we are creating the index with the 2 underscores in the beginning which is preventing the index to be dropped directly.
I tried doing the same but was getting parser exceptions -

0: jdbc:phoenix:localhost> DROP INDEX __CDC__CDCTABLE ON DATATABLE; Error: ERROR 601 (42P00): Syntax error. Unexpected char: '_' (state=42P00,code=601) org.apache.phoenix.exception.PhoenixParserException: ERROR 601 (42P00): Syntax error. Unexpected char: '_' at org.apache.phoenix.exception.PhoenixParserException.newException(PhoenixParserException.java:33)

Copy link
Contributor

Choose a reason for hiding this comment

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

underscores in the beginning which is preventing the index to be dropped directly.

You can still drop it by putting double quotes around the index name.

@virajjasani virajjasani changed the title PHOENIX-7054 DROP CDC Implementation PHOENIX-7074 DROP CDC Implementation Oct 17, 2023
return dropTable(schemaName, indexName, parentTableName, PTableType.INDEX, statement.ifExists(), false, false);
} catch (SQLException e) {
throw new SQLExceptionInfo.Builder(SQLExceptionCode.fromErrorCode(e.getErrorCode()))
.setTableName(statement.getCdcObjName().getName()).build().buildException();
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing setRootCause(). The same applies to create CDC as well, which I will fix in the next PR.

if (CDCUtil.isACDCIndex(
ExecutableDropIndexStatement.this.getIndexName().getName())) {
throw new SQLExceptionInfo.Builder(CANNOT_DROP_CDC_INDEX).setTableName(
ExecutableDropIndexStatement.this.getIndexName().getName())
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: ExecutableDropIndexStatement.this.getIndexName().getName() could be assigned to a local variable to avoid repetition.

Copy link
Author

Choose a reason for hiding this comment

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

@haridsv , updated

Copy link
Contributor

@haridsv haridsv left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@virajjasani virajjasani left a comment

Choose a reason for hiding this comment

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

+1

@virajjasani virajjasani merged commit 5d3fd40 into apache:PHOENIX-7001-feature Dec 4, 2023
1 of 2 checks passed
haridsv added a commit to haridsv/phoenix that referenced this pull request Mar 29, 2024
This commit includes a squash of all the below changes from the PRs apache#1662, apache#1694,
down by removing non-functional changes for the ease of review. On top of it,
the changes have been reworked for the changed state of master, especially the
new submodules.

* 4c9827a Hari.. PHOENIX-7008 Shallow grammar support for CREATE CDC (apache#1662)
* e5220e0 TheN..  PHOENIX-7054 Shallow grammar support for DROP CDC and ALTER CDC  (apache#1694)
* 581e613 Hari.. PHOENIX-7008 Implementation for CREATE CDC  (apache#1681)
* e2ef886 Hari.. PHOENIX-7008 Tweaks, fixes and additional test coverage for CREATE CDC (apache#1703)
* 5d3fd40 TheN.. PHOENIX-7074 DROP CDC Implementation (apache#1713)
* 7420443 Hari.. PHOENIX-7014: Query compiler/optimizer changes along with some PHOENIX-7015 changes (apache#1766)
* da6ddad Kadi.. Add an extra delete mutation for CDC
* 93d586e Kadi.. Add an extra delete mutation during rebuild for CDC index
* f07898f Hari.. PHOENIX-7008: Addressing Jira spec and review feedback changes (apache#1802)
* 87a2ea1 Hari.. PHOENIX-7008: Fix for parser gap and fix for failing test (apache#1812)
* e395780 TheN.. PHOENIX-7015 Implementing CDCGlobalIndexRegionScanner (apache#1813)

Co-authored-by: Saurabh Rai <saurabh.rai@salesforce.com>
haridsv added a commit to haridsv/phoenix that referenced this pull request Mar 29, 2024
…ture

This commit includes a squash of all the below changes from the PRs apache#1662, apache#1694,
down by removing non-functional changes for the ease of review. On top of it,
the changes have been reworked for the changed state of master, especially the
new submodules.

* 4c9827a Hari.. PHOENIX-7008 Shallow grammar support for CREATE CDC (apache#1662)
* e5220e0 TheN..  PHOENIX-7054 Shallow grammar support for DROP CDC and ALTER CDC  (apache#1694)
* 581e613 Hari.. PHOENIX-7008 Implementation for CREATE CDC  (apache#1681)
* e2ef886 Hari.. PHOENIX-7008 Tweaks, fixes and additional test coverage for CREATE CDC (apache#1703)
* 5d3fd40 TheN.. PHOENIX-7074 DROP CDC Implementation (apache#1713)
* 7420443 Hari.. PHOENIX-7014: Query compiler/optimizer changes along with some PHOENIX-7015 changes (apache#1766)
* da6ddad Kadi.. Add an extra delete mutation for CDC
* 93d586e Kadi.. Add an extra delete mutation during rebuild for CDC index
* f07898f Hari.. PHOENIX-7008: Addressing Jira spec and review feedback changes (apache#1802)
* 87a2ea1 Hari.. PHOENIX-7008: Fix for parser gap and fix for failing test (apache#1812)
* e395780 TheN.. PHOENIX-7015 Implementing CDCGlobalIndexRegionScanner (apache#1813)

Co-authored-by: Saurabh Rai <saurabh.rai@salesforce.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants