Add In Clause handling in json indexed col (Attr) #6147
Conversation
bowenxia
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
bowenxia
changed the title
common/pinot/pinotQueryValidator.go
Outdated
| @@ -71,7 +71,7 @@ func (qv *VisibilityQueryValidator) ValidateQuery(whereClause string) (string, e | |||
|
|
|||
| stmt, err := sqlparser.Parse(placeholderQuery) | |||
| if err != nil { | |||
| return "", &types.BadRequestError{Message: "Invalid query."} | |||
| return "", &types.BadRequestError{Message: "Invalid query." + err.Error()} | |||
There was a problem hiding this comment.
Print error message here for a better debugging experience.
There was a problem hiding this comment.
I'm unsure if it is a good idea since this error will be propagated to the customers. Do we want to expose which DB or which tables we use for customers? We can log it for debugging purposes while exposing only a generic message to the client.
Or, we can extract details about which arguments in the query are invalid to help us understand what is wrong. See *json.UnmarshalTypeError is an example that can point to the exact failure while not exposing the whole thing.
There was a problem hiding this comment.
It's actually a sql expression parsing error.
It will have something like:
"Invalid query.syntax error at position 53 near 'select'"
"Invalid query.syntax error at position 38 near 'sql'"
Doesn't expose DB details or tables.
There was a problem hiding this comment.
Let's add a space after the dot or change it to colon. query.syntax is a bit confusing
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Pull Request Test Coverage Report for Build 019051d7-b549-4c26-b296-87c3bc87a437Details
💛 - Coveralls |
| values[i] = "''" + string(sqlVal.Val) + "''" | ||
| } | ||
|
|
||
| return fmt.Sprintf("JSON_MATCH(Attr, '\"$.%s\" IN (%s)') or JSON_MATCH(Attr, '\"$.%s[*]\" IN (%s)')", |
There was a problem hiding this comment.
This query is amazing, I got help from gpt to understand :)
Pull Request Test Coverage Report for Build 0190567b-a424-4d32-a57e-d52b447b9cb0Details
💛 - Coveralls |
What changed?
Added
INClause handling for json indexed col (Attr)And unit test with 100% coverage of the new function.
Why?
One customer complained that they had error when using
INClause after we migrated their domain to Pinot. Because previously ES supported that.Pinot DOES supported this for all other system keys; but in a json indexed col like
ATTR(we stored this column in a json format), we need to handleINclause in a different way.Sample query:
if we input:
BinaryChecksums IN ("uDeploy:e6b658fc4ae98445a356d2218316081f7113fcdf")It would be processed to:
and JSON_MATCH(Attr, '"$.BinaryChecksums" IN (''uDeploy:e6b658fc4ae98445a356d2218316081f7113fcdf'')') or JSON_MATCH(Attr, '"$.BinaryChecksums[*]" IN (''uDeploy:e6b658fc4ae98445a356d2218316081f7113fcdf'')')How did you test it?
unit test
Potential risks
Release notes
Documentation Changes