-
Notifications
You must be signed in to change notification settings - Fork 317
fix: delimited column identifier for dynamic fields #1127
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: b57fc16 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
@tombokombo is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Hi @wrn14897 , could you please take a look? |
Hello, I'm running custom build with this patch in production for weeks, could somebody please take a look. Thanks. |
Hey @tombokombo, I will take a look at this PR this week. Thanks! |
return SqlString.format(`toString(?)='?'`, [ | ||
SqlString.raw(valueExpr), | ||
// remove double quotes if present and escape single quotes | ||
return SqlString.format(`toString(??)='?'`, [ |
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.
I suspect this could break the alias expression. We should check if tests pass after this PR (#1231)
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.
ok, so we are waiting till PR (#1231) becomes merged ?
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.
correct
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.
@wrn14897 let's try tests :)
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.
@wrn14897 my local build passed tests and this patch is still needed
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.
@tombokombo can you resolve the conflicts?
Hi,
in dynamic fields there could be identifier like
log.ctxt.request.Symfony\Component\HttpFoundation\Request
(real world example from php logs).Search query will be
toString(log.ctxt.request.Symfony\Component\HttpFoundation\Request)=
and backend will return 400 Bad request complaining about syntax error.Escaping identifiers with
??
which is shorthand to SqlString.escapeId() solves that problem.I've removed value length conditions as well. They are no more needed as was mentioned in my previous PR #1064
Fixed another issue with single quotes which was introduced by #1064