-
Notifications
You must be signed in to change notification settings - Fork 318
fix: Fix sidebar when selecting JSON property #1231
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
🦋 Changeset detectedLatest commit: cc6d4c3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
The latest updates on your projects. Learn more about Vercel for GitHub.
|
2728847
to
eb634be
Compare
eb634be
to
52388fe
Compare
E2E Test Results✅ All tests passed • 25 passed • 3 skipped • 205s
|
52388fe
to
81805af
Compare
81805af
to
99c2e3f
Compare
// Currently we can't distinguish null or 'null' | ||
if (value === 'null') { | ||
return SqlString.format(`isNull(??)`, [column]); | ||
return SqlString.format(`isNull(?)`, [SqlString.raw(valueExpr)]); |
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 now ensures that the original expression, rather than the alias, is used. This matches the behavior elsewhere in this function and fixes another caused of sidebar loading failures for JSON types.
99c2e3f
to
30dbf17
Compare
We can ship patches for now. However, it looks like someone has already built the ClickHouse dialect and might get merged to the upstream. I think we should give it a try. In the long term, we should focus on improving this dialect instead of relying on the postgres one. |
(Accidentally closed, apologies) |
Yeah I saw that! I upvoted the comments asking the maintainer to merge the ClickHouse support 🤞 It would be nice to be able to contribute to that instead of building these workarounds. |
packages/common-utils/src/utils.ts
Outdated
} | ||
|
||
/** Returns true if the given expression is a JSON expression, eg. `col.key.nestedKey` or "json_col"."key" */ | ||
const isJsonExpression = (expr: string) => { |
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.
Are '.' or 1.5 valid JSON? This method returns true in these cases, but I’m not sure if that affects other methods.
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.
Thanks, updated isJsonExpression
so that it does not recognize .
or 1.5
as JSON expressions
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.
Could you add a few more unit tests for this method?
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.
Added some more unit tests 👍
const { sqlWithReplacements, replacements: jsonReplacementsToExpressions } = | ||
replaceJsonExpressions(sql); |
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.
Is it possible to enable this only when node-sql-parser isn't able to parse the sql or if it contains JSON syntax?
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.
Unfortunately that isn't straightforward either, because node-sql-parser doesn't error on all JSON - sometimes it just parses the JSON incorrectly as schema.table.column
or table.column
. And determining if it contains json syntax would probably have to make use of the same functions being used here to find and replace the JSON.
* | ||
* Note - This function does not distinguish between json references and `table.column` references - both are returned. | ||
*/ | ||
export function findJsonExpressions(sql: string) { |
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 have a lot of confidence that this will work for all edge cases, but I’m okay with shipping the patch until we fix the parser
0dadd7e
to
8d87e0a
Compare
@wrn14897 I also confirmed that the forked node-sql-parser with ClickHouse support does not yet support JSON 😞 We could consider contributing that support or try to get ClickHouse Analyzer in a usable state. 100% agreed that it would be best to have a working ClickHouse SQL parser instead of a bunch of patches on node-sql-parser |
394636f
to
dacb30d
Compare
Closes HDX-2042
Closes HDX-2524
Closes HDX-2307
Closes #1010
Summary
This PR fixes errors that occurred when attempting to open the sidebar by clicking a log table row using a JSON logs table schema.
The error was caused by
node-sql-parser
throwing exceptions when parsing SQL with JSON Expressions, resulting in HyperDX being unable to extract aliases from the SQL. In the long term, we'll want to have a true ClickHouse SQL parser. In the short term, this is fixed by:Testing
(All of the following use a JSON schema)
Before
When selecting a JSON column with an alias
When filtering by a JSON column and using an alias on a non-JSON property
After
When selecting a JSON column with an alias
When filtering by a JSON column and using an alias on a non-JSON property