Web console: improve SQL autocomplete and add JSON autocomplete #18126
Web console: improve SQL autocomplete and add JSON autocomplete #18126vogievetsky merged 44 commits intoapache:masterfrom
Conversation
There was a problem hiding this comment.
oh man, that is a lot of API to review, i'll be real here and admit that so far i just scanned it and picked on the things that stuck out.
what are we going for here, is the thinking that mostly correct is better than nothing? or should we try to review this more closely to ensure it is accurate? what is the plan for maintenance, should PR authors update this when they update stuff? I don't have many strong opinions either way yet, just some thoughts that come to mind after looking over this.
All that said, this is pretty cool though 😎
| isObject: true, | ||
| condition: obj => obj.type === 'index_parallel' || obj.type === 'index', | ||
| completions: [ | ||
| { value: 'firehose', documentation: 'Legacy data input (deprecated)' }, |
There was a problem hiding this comment.
this is gone, not just deprecated
| completions: [ | ||
| { value: 'uris', documentation: 'List of HTTP/HTTPS URIs' }, | ||
| { value: 'httpAuthenticationUsername', documentation: 'HTTP authentication username' }, | ||
| { value: 'httpAuthenticationPassword', documentation: 'HTTP authentication password' }, |
There was a problem hiding this comment.
there are a couple missing ones here, systemFields and requestHeaders though the latter isn't documented either
There was a problem hiding this comment.
I'll add systemFields and will add requestHeaders when it is documented
| { | ||
| path: '$', | ||
| isObject: true, | ||
| condition: obj => obj.queryType !== 'dataSourceMetadata', |
There was a problem hiding this comment.
afaik, everything has a context and intervals, even dataSourceMetadata
There was a problem hiding this comment.
Huh, it is not documented here: https://druid.apache.org/docs/latest/querying/datasourcemetadataquery/ I even double cheched this one. It kind of makes sense for dataSourceMetadata not to support intervals since it is reporting back the max ingested time.
| { | ||
| path: '$.filter.type', | ||
| completions: [ | ||
| { value: 'selector', documentation: 'Matches a specific dimension value' }, |
There was a problem hiding this comment.
this is missing the range filter and typed in filter (which i just realized isn't documented yet, from #16039)
There was a problem hiding this comment.
I am sticking only to the documented stuff for now
| value: 'populateResultLevelCache', | ||
| documentation: 'Whether to populate result level cache', | ||
| }, | ||
| { value: 'bySegment', documentation: 'Return results by segment' }, |
There was a problem hiding this comment.
does this still break the results if you actually set it?
There was a problem hiding this comment.
bySegment: true should work :-)
| * For automatic compaction, set the type to autocompact." | ||
| */ | ||
| function isSupervisorType(type: string): boolean { | ||
| return type === 'kafka' || type === 'kinesis' || type === 'rabbit' || type === 'autocompact'; |
| { value: 'timestampSpec', documentation: 'Timestamp specification of data in segments' }, | ||
| { value: 'queryGranularity', documentation: 'Query granularity of data in segments' }, | ||
| { value: 'aggregators', documentation: 'List of aggregators usable for metric columns' }, | ||
| { value: 'rollup', documentation: 'Whether the segments are rolled up' }, |
| export const COMPACTION_CONFIG_COMPLETIONS: JsonCompletionRule[] = [ | ||
| // Root level properties | ||
| { | ||
| path: '$', |
There was a problem hiding this comment.
this isn't really correct, after #17803 there is a "type" for a datasource compaction config, with the old spec type being "inline" and the one added in that PR is "catalog"
There was a problem hiding this comment.
I'll add that when the docs are up
|
Thank you for the review and feedback @clintropolis in general I have taken the approach of only going after the documented properties. I also think of these JSON completions as best effort. It is ok for them to miss stuff. Even if they are totally wrong it is not a blocker (even though it is best to fix) as the completions are purely cosmetic. As for updating I expect the completions to be updated like like the properties for rendering all the AutoForms. Ideally done by the dev adding a new property in their PR but more often done after the fact once that property is exposed and documented |
clintropolis
left a comment
There was a problem hiding this comment.
approving because i think this does more help than harm, but I still am a bit wary about long term maintainability. In its current state it seems like maybe we just have to like periodically ask claude (or some similar bot) to refresh it to match the docs? It would be really nice to like consolidate the common definitions across the completion files somehow so that human devs could potentially proactively add or adjust stuff here, but not a blocker.
Still, neat overall 🤘
| 'DOW', | ||
| 'DOW', |
There was a problem hiding this comment.
do dupes matter? also this doesn't work in druid (and a bunch of others in this list), though I guess calcite recognizes it since i see it in the parser stuff 🤷
| path: '$.spec.dataSchema.dimensionsSpec.useSchemaDiscovery', | ||
| completions: [ | ||
| { value: 'true', documentation: 'Enable automatic type detection and schema discovery' }, | ||
| { value: 'false', documentation: 'Use manual dimension specification (default)' }, |
There was a problem hiding this comment.
this is perhaps misleading since you can still discover dimensions by leaving the list empty OR by specifying includeAllDimension as true.. though maybe that makes it even more confusing..
| { value: 'true', documentation: 'Create bitmap index (default, faster filtering)' }, | ||
| { value: 'false', documentation: 'No bitmap index (saves storage)' }, |
There was a problem hiding this comment.
should we indicate that this only works for string type dimensions?
| { value: 'sorted_array', documentation: 'Sort multi-values (default)' }, | ||
| { value: 'sorted_set', documentation: 'Sort and deduplicate multi-values' }, | ||
| { value: 'array', documentation: 'Keep multi-values as-is' }, |
There was a problem hiding this comment.
should we mention this only works for string type dimensions?
|
|
||
| // Aggregation types | ||
| { | ||
| path: '$.aggregations.[].type', |
There was a problem hiding this comment.
i've noticed this repeated quite a lot across the completions (and also some other common components like filters), is there a way to share it perhaps? fine to do as a follow-up, but sharing common elements across query, ingestion, compaction, etc would help make this a ton more maintainable
|
I think the feedback all makes sense. I also think this PR makes things a lot better. In a follow up PR I will change this behaviour to use json-schema which is a standard and allows for recursive schema definition |
Fixes #17628
Made the SQL autocomplete much smarter
(note how it is not trying to auto-complete functions and other nonsense)
Also added autocomplete to JSON native queries and to all the JSON entry fields