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-7507 Update ROW_KEY_MATCHER column type to be VARBINARY_ENCODED #2060

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jpisaac
Copy link
Contributor

@jpisaac jpisaac commented Jan 21, 2025

No description provided.

case Types.VARBINARY:
pkTypeStr = "VARBINARY";
break;
case 9000:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: PDataType.VARBINARY_ENCODED_TYPE

@@ -183,6 +194,15 @@ private Object getData(PDataType type) {
case Types.TIMESTAMP:
//pkTypeStr = "TIMESTAMP";
return new Timestamp(System.currentTimeMillis() + rnd.nextInt(50000));
case Types.VARBINARY:
// pkTypeStr = "VARBINARY";
case 9000:
Copy link
Contributor

Choose a reason for hiding this comment

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

same here: PDataType.VARBINARY_ENCODED_TYPE

RandomStringUtils.randomAlphanumeric(25).getBytes(),
Bytes.toBytes(rnd.nextInt(50000)),
Bytes.toBytes(Math.floor(rnd.nextInt(50000) * rnd.nextDouble())));
LOGGER.info("***************************************************************************");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove logging

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.

Left few nits

pkType3Str, pkOrders[0].name(), pkOrders[1].name(), pkOrders[2].name(),
baseTableName, partitionName));
baseTableName, partitionName);
LOGGER.info(String.format("Created global view %s", globalViewSQL));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: LOGGER.info("Created global view {}", globalViewSQL)

String.format(GLOBAL_INDEX_TEMPLATE, indexNamePrefix, globalViewName));
String globalViewIndexSQL =
String.format(GLOBAL_INDEX_TEMPLATE, indexNamePrefix, globalViewName);
LOGGER.info(String.format("Created global view index %s", globalViewIndexSQL));
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, String.format not needed

String.format(VIEW_WITH_PK_TEMPLATE, tenantViewName, globalViewName,
getWhereClause(pkNames, pkTypes), tenantViewOptions));
getWhereClause(pkNames, pkTypes), tenantViewOptions);
LOGGER.info(String.format("Created tenant view (WITH_PK) %s", viewWithPKSQL));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: LOGGER.info("Created tenant view (WITH_PK) {}", viewWithPKSQL)

tenantViewOptions));
String viewWithoutPKSQL = String.format(VIEW_WO_PK_TEMPLATE, tenantViewName, globalViewName,
tenantViewOptions);
LOGGER.info(String.format("Created tenant view ((WO_PK)) %s", viewWithoutPKSQL));
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

));

// View TTL is set to 10s => 10000 ms
//globalViewOptions.setTableProps(String.format("TTL=%d", viewTTL));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove commented

tenantViewOptions.setTableProps(String.format("TTL=%d", viewTTL));

// OtherOptions otherOptions = new OtherOptions();
// otherOptions.setTableCFs(asList(null, null, null));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove commented

int viewTTL = VIEW_TTL_10_SECS;
// Define the test schema.
// 1. Table with columns => (ORG_ID, KP, COL1, COL2, COL3), PK => (ORG_ID, KP)
// 2. GlobalView with columns => (ID, COL4, COL5, COL6), PK => (ID)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update comments with actual column names

Copy link
Contributor

@kadirozde kadirozde left a comment

Choose a reason for hiding this comment

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

+1, Thanks!

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.

@jpisaac
Copy link
Contributor Author

jpisaac commented Jan 23, 2025

+1, pending build results from https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-2060/2/

The backward compatibility test failures are due to VARBINARY_ENCODED type not being in other branches (for e.g 5.1.*) @virajjasani

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