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

list table names with snapshot #566

Merged
merged 3 commits into from
Jan 9, 2024
Merged

Conversation

stevie9868
Copy link
Contributor

Also makes list table names using the AS OF SYSTEM TIME semantics.

  1. Basically reuse the previous AS OF SYSTEM TIME implementation to include used case for listing table names
  2. Move some unit tests to functional tests related to list table names since AS OF SYSTEM TIME is only supported in crdb

for (String tableName : polarisStoreService.getTables(name.getDatabaseName(), tableFilter)) {
for (String tableName : polarisStoreService.getTables(name.getDatabaseName(),
tableFilter,
1000)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this constant in the code be made into a constant variable

final List<String> tableNames = polarisStoreService.getTables(
name.getDatabaseName(),
"",
1000);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since names are smaller than the objects themselves, it might be more efficient to make the constant larger for names, maybe we can try 5k or 10k

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this one was missed, can you update it to read from config as well

List<PolarisTableEntity> findAllTablesByDbNameAndTablePrefix(
String dbName, String tableNamePrefix, int pageSize);
List<?> findAllTablesByDbNameAndTablePrefix(
String dbName, String tableNamePrefix, int pageSize, boolean selectAll);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The selectAll name could be improved since the method name already has findAll so it could seem like it might not return the whole list, this could be renamed instead to selectNames or something similar

@insyncoss insyncoss self-requested a review January 9, 2024 03:39
Copy link
Collaborator

@insyncoss insyncoss left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments

@stevie9868 stevie9868 merged commit 3f55f9f into master Jan 9, 2024
3 checks passed
@insyncoss insyncoss deleted the list_table_names_with_Snapshot branch February 2, 2024 01:45
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