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

Introduce new methods projected & exceptColumns take string varargs in QueryAssertions #16609

Merged

Conversation

krvikash
Copy link
Contributor

@krvikash krvikash commented Mar 17, 2023

Description

For better readability, replace projected(int... columns) with projected(String... columnNamesToInclude) and introduce exceptColumns(String... columnNamesToExclude) leveraging MaterializedResult.getColumnNames

@cla-bot cla-bot bot added the cla-signed label Mar 17, 2023
@krvikash krvikash requested review from ebyhr and findepi March 17, 2023 12:45
@krvikash krvikash self-assigned this Mar 17, 2023
@github-actions github-actions bot added bigquery BigQuery connector iceberg Iceberg connector labels Mar 17, 2023
@findepi findepi added test no-release-notes This pull request does not require release notes entry labels Mar 17, 2023
@krvikash krvikash force-pushed the remove-use-of-deprecated-projected-method branch from cd8fb28 to caeaae4 Compare March 21, 2023 10:10
@krvikash krvikash force-pushed the remove-use-of-deprecated-projected-method branch from caeaae4 to 16ae772 Compare March 21, 2023 10:18
@krvikash krvikash changed the title Replace projected with exceptColumns method in QueryAssertions Introduce new methods projected & exceptColumns take string varargs in QueryAssertions Mar 21, 2023
@krvikash krvikash force-pushed the remove-use-of-deprecated-projected-method branch from 16ae772 to 6a8b280 Compare March 22, 2023 06:41
@krvikash
Copy link
Contributor Author

CI is failing with #16315

@krvikash krvikash force-pushed the remove-use-of-deprecated-projected-method branch from 6a8b280 to d74a86a Compare March 22, 2023 16:52
@krvikash
Copy link
Contributor Author

Rebased with master.

Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

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

"Capture column names in LocalQueryRunner"

@@ -892,6 +893,9 @@ private MaterializedResultWithPlan executeInternal(Session session, @Language("S
}

verify(builder.get() != null, "Output operator was not created");
if (plan.getRoot() instanceof OutputNode outputNode) {
Copy link
Member

Choose a reason for hiding this comment

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

can this condition be false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All test cases are passing, So I assume check is not required.

@krvikash krvikash force-pushed the remove-use-of-deprecated-projected-method branch from d74a86a to 0a341fb Compare March 23, 2023 09:51
@krvikash krvikash force-pushed the remove-use-of-deprecated-projected-method branch from 0a341fb to d30528d Compare March 23, 2023 10:47
@krvikash krvikash force-pushed the remove-use-of-deprecated-projected-method branch from d30528d to 306b2c4 Compare March 23, 2023 17:40
For better readability, replace projected(int... columns) with
projected(String... columnNamesToInclude) and introduce
exceptColumns(String... columnNamesToExclude) leveraging
MaterializedResult.getColumnNames
@findepi findepi force-pushed the remove-use-of-deprecated-projected-method branch from 306b2c4 to 3d41fff Compare March 24, 2023 09:24
@findepi
Copy link
Member

findepi commented Mar 24, 2023

Fixed unnecessary use of a linkedhashset

@findepi findepi merged commit dc1f22f into trinodb:master Mar 24, 2023
@github-actions github-actions bot added this to the 411 milestone Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bigquery BigQuery connector cla-signed iceberg Iceberg connector no-release-notes This pull request does not require release notes entry test
Development

Successfully merging this pull request may close these issues.

3 participants