You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "abhishekrb19 (via GitHub)" <gi...@apache.org> on 2024/03/19 23:58:50 UTC
[PR] Parameterize segment IDs (druid)
abhishekrb19 opened a new pull request, #16174:
URL: https://github.com/apache/druid/pull/16174
#### Issue:
The `markUsed` API accepts either an `interval` or `segmentIds`. When using segment IDs, the underlying query uses an `IN` clause with the user-provided values inlined directly in the query instead of being parameterized.
#### Fix:
This patch fixes that by adding the following utilities specifically for queries that contain an `IN` clause:
- `getParameterizedInConditionForColumn()`
- `bindColumnValuesToQueryWithInCondition()`
#### Other related changes:
Renamed the following functions for consistency and code hygiene:
- `appendConditionForIntervalsAndMatchMode()` to `getConditionForIntervalsAndMatchMode()`. Pass only select arguments to the function.
- `bindQueryIntervals()` to `bindIntervalsToQuery()`
This PR has:
- [x] been self-reviewed.
- [ ] a release note entry in the PR description.
- [x] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [x] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Parameterize segment IDs (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on PR #16174:
URL: https://github.com/apache/druid/pull/16174#issuecomment-2014089434
> What kind of validation and exception did you have in mind? Currently, the code doesn't perform any type checks for the columns.
I should think that JDBI would throw some exception if we try to bind am invalid string which contains, say a substring like `'); DROP TABLE abc;`.
We could try to verify that behaviour. By the way, this is optional and need not block this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Parameterize segment IDs (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on PR #16174:
URL: https://github.com/apache/druid/pull/16174#issuecomment-2014165962
Ah, I see. The JDBI library doesn't check for "illegal" values for the named parameters as it's contextual. I ran a test with such values and it seems the library just treats them as any other string literal. That said, the binding itself makes the query safe to execute and I think if we want to detect illegal values or perform semantic validation, it has to be done at a layer on top (I'm not sure if that check can be full-proof though).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Parameterize segment IDs (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on PR #16174:
URL: https://github.com/apache/druid/pull/16174#issuecomment-2014261583
> That said, the binding itself makes the query safe to execute
Yeah, that's a fair point. Thanks for the clarification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Parameterize segment IDs (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on PR #16174:
URL: https://github.com/apache/druid/pull/16174#issuecomment-2014068395
@kfaraz, thanks for taking a look!
> Thanks for the change, @abhishekrb19 . This is very useful!
>
> It would be nice to have a test to verify that the code throws an exception when passing illegal values for the version or any other column.
What kind of validation and exception did you have in mind? Currently, the code doesn't perform any type checks for the columns.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Parameterize segment IDs (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 merged PR #16174:
URL: https://github.com/apache/druid/pull/16174
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org