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

Minor : Fix exception in search due to exception in database.displayN… #18290

Merged
merged 2 commits into from
Oct 16, 2024

Conversation

sonika-shah
Copy link
Contributor

Describe your changes:

Minor : Fix exception in search due to exception in database.displayName and databaseSchema.aggregation

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

Copy link
Contributor

The Python checkstyle failed.

Please run make py_format and py_format_check in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Python code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

1 similar comment
Copy link
Contributor

The Python checkstyle failed.

Please run make py_format and py_format_check in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Python code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

Comment on lines +1301 to +1308
AggregationBuilders.terms("database.name.keyword")
.field("database.name.keyword")
.size(MAX_AGGREGATE_SIZE));
searchSourceBuilder.aggregation(
AggregationBuilders.terms("databaseSchema.name.keyword")
.field("databaseSchema.name.keyword")
.size(MAX_AGGREGATE_SIZE));
return addAggregation(searchSourceBuilder);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do the aggregation on the fullyQualifiedName? Using name I believe if we have service.database.schema and otherService.database.schema then they'll both get aggregated under the same if we use name I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @TeddyCr thanks for pointing your concern is valid, but for our usage we are not dependent
only on a single aggregation while getting result, we check the condition with other aggregations as well
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have tried to replicate the same scenario with same database and schema, seems to be working fine.
Also will be removing the support for aggregations on name.keyword, as in most scenario we are moving to show results based on displayName

image

Comment on lines +1285 to +1293
searchSourceBuilder.aggregation(
AggregationBuilders.terms("database.name.keyword")
.field("database.name.keyword")
.size(MAX_AGGREGATE_SIZE));
searchSourceBuilder.aggregation(
AggregationBuilders.terms("databaseSchema.name.keyword")
.field("databaseSchema.name.keyword")
.size(MAX_AGGREGATE_SIZE));
return addAggregation(searchSourceBuilder);
Copy link
Contributor

@TeddyCr TeddyCr Oct 16, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor

The Python checkstyle failed.

Please run make py_format and py_format_check in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Python code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

@harshach harshach merged commit 9dbe7e3 into 1.5.7 Oct 16, 2024
6 of 16 checks passed
@harshach harshach deleted the show-explore-tree-display-2-for-1.5.7 branch October 16, 2024 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend safe to test Add this label to run secure Github workflows on PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants