Skip to content

Conversation

@palashc
Copy link
Contributor

@palashc palashc commented Mar 19, 2024

No description provided.

@palashc
Copy link
Contributor Author

palashc commented Apr 7, 2024

@shahrs87 Let me know if we can get this one in now?

Copy link
Contributor

@shahrs87 shahrs87 left a comment

Choose a reason for hiding this comment

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

some changes.

Copy link
Contributor

@shahrs87 shahrs87 left a comment

Choose a reason for hiding this comment

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

Some comments.

@palashc palashc requested a review from shahrs87 April 18, 2024 16:55
private static final Logger LOGGER = LoggerFactory.getLogger(MetaDataCachingIT.class);
private final Random RAND = new Random(11);

private boolean isLastDDLTimestampValidationEnabled = config.getBoolean(
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of duplicating this line in 3 different tests, do you mind creating a util method in ValidateLastDDLTimestampUtil something like getValidateLastDdlTimestampEnabled(Configuration config) similar to getValidateLastDdlTimestampEnabled(PhoenixConnection connection)

public static ColumnResolver getResolver(SingleTableStatement statement, PhoenixConnection connection, Map<String, UDFParseNode> udfParseNodes)
throws SQLException {
SingleTableColumnResolver visitor = new SingleTableColumnResolver(connection, statement.getTable(), true, 0, udfParseNodes);
SingleTableColumnResolver visitor
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that we are calling this resolver from MetaDataClient#createIndex and CreateIndexCompiler#compile method. So we want to bypass cache for createIndex and resolve table always. So lets rename this method to getResolverForCreateIndex and add a comment on why we are setting alwaysHitServer to true.

Copy link
Contributor

@shahrs87 shahrs87 left a comment

Choose a reason for hiding this comment

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

Other than 2 minor comments, +1 ltgm. Thank you @palashc for the change.

@palashc
Copy link
Contributor Author

palashc commented Apr 18, 2024

@shahrs87 Latest test report - https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1859/22/testReport/
UpsertSelectIT.testUpsertSelectForAgg passes locally.

@shahrs87 shahrs87 merged commit 01a28f1 into apache:PHOENIX-6883-feature Apr 19, 2024
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.

2 participants