-
Notifications
You must be signed in to change notification settings - Fork 246
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
fix: autocomplete for lucene column values #720
Conversation
🦋 Changeset detectedLatest commit: 7f06fa2 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
formatKeyValPair: (key: string, value: string) => string; | ||
} | ||
|
||
export function useAutoCompleteOptions( |
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.
New hook to do facilitate all of the autocomplete options and inputs. The formatter arg is an interface class that'll be different for lucene and sql
keys, | ||
limit, | ||
disableRowLimit, | ||
}: { | ||
chartConfig: ChartConfigWithDateRange; | ||
chartConfigs: ChartConfigWithDateRange | ChartConfigWithDateRange[]; |
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.
Renamed for consistency with tableConnections
in useAllFields
( | ||
await Promise.all( | ||
chartConfigsArr.map(chartConfig => | ||
metadata.getKeyValues({ |
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.
Refactored to support multiple chart configs
094b997
to
3d9f864
Compare
databaseName, | ||
tableName, | ||
}, | ||
timestampValueExpression: '', |
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.
Don't we want to add time range filtering? When users perform a search, we can narrow down the time range to speed up the query
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.
Likely, what do you think is appropriate? Hardcoding some expression (last 24 hours) or prop drill it in from somewhere?
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.
Hmm...now when I think about this again. Its probably not straightforward to bypass the time range config. Given we have a limit on scanning rows, this should be fine for now.
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 is the coolest feature imo 💪 💪 💪 Can't wait to ship it
|
||
export class LuceneLanguageFormatter implements ILanguageFormatter { | ||
formatFieldValue(f: Field): string { | ||
return f.path.join('.'); |
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.
no need to fix it for now but I'm curious what happened if the key has '.' in it
Added autocomplete for potential search values for lucene where clauses.
Added testing for useAutoCompleteOptions and useGetKeyValues.
Ref: HDX-1509