-
Notifications
You must be signed in to change notification settings - Fork 2
Do not show datasources which have spotter disabled #70
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
Conversation
Summary of ChangesHello @shikharbsar, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the display logic for data sources by introducing a new filtering criterion. The primary goal is to ensure that only data sources explicitly enabled for AI answer generation (spotter) are presented to the user, irrespective of their specific type. This modification enhances the accuracy and relevance of available data sources for AI-driven functionalities and is fully supported by updated unit tests that validate the new filtering behavior. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates the data source filtering logic to exclude sources where AI answer generation is disabled. The implementation is mostly correct, but I've suggested a more robust filtering condition to handle cases where the aiAnswerGenerationDisabled
property might be missing from the API response. Additionally, I've pointed out a misleading comment in the test suite that should be corrected for clarity.
.filter(d => d.metadata_header.type === "WORKSHEET" || d.metadata_header.subType === "WORKSHEET") | ||
// Tables can also be used for spotter now | ||
//.filter(d => d.metadata_header.type === "WORKSHEET" || d.metadata_header.subType === "WORKSHEET") | ||
.filter(d => d.metadata_header.aiAnswerGenerationDisabled === false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current filter d.metadata_header.aiAnswerGenerationDisabled === false
will exclude datasources where aiAnswerGenerationDisabled
is undefined
or null
. If the absence of this property implies that AI answer generation is enabled, this filter is too strict and might hide datasources that should be visible.
A more robust approach would be to check for explicit disablement, like so: d.metadata_header.aiAnswerGenerationDisabled !== true
. This ensures that only datasources explicitly marked as disabled are filtered out, while those with false
, null
, or undefined
values are correctly included.
To ensure this behavior is covered, you could also add a test case in test/thoughtspot/thoughtspot-service.spec.ts
for a datasource where aiAnswerGenerationDisabled
is missing.
.filter(d => d.metadata_header.aiAnswerGenerationDisabled === false) | |
.filter(d => d.metadata_header.aiAnswerGenerationDisabled !== true) |
}, | ||
{ | ||
metadata_header: { | ||
type: 'LOGICAL_TABLE', // This should be filtered out due to aiAnswerGenerationDisabled: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is misleading. It states that this data source should be filtered out, but aiAnswerGenerationDisabled
is false
, and this data source (lt2
) is correctly included in the test's expected results. The comment appears to be a copy-paste error from the object above. Please correct or remove it to avoid confusion.
type: 'LOGICAL_TABLE', // This should be filtered out due to aiAnswerGenerationDisabled: true | |
type: 'LOGICAL_TABLE', // This is included because aiAnswerGenerationDisabled is false |
No description provided.