You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "xiangfu0 (via GitHub)" <gi...@apache.org> on 2026/08/21 03:28:00 UTC
[PR] Add safe SQL quoting for typed table names [pinot]
xiangfu0 opened a new pull request, #19327:
URL: https://github.com/apache/pinot/pull/19327
## Summary
- add `TableNameBuilder.quoteTableNameWithType` for reusable typed-table SQL quoting
- quote optional database and table components separately
- escape embedded double quotes while preserving the existing non-whitespace character contract
- reject malformed qualification and missing canonical table-type suffixes
## Motivation
Callers that construct internal SQL currently need to quote typed table resource names themselves. Quoting a whole database-qualified name changes its meaning, while an ASCII allowlist would reject table names Pinot currently permits. This helper centralizes the component-aware and injection-safe behavior in `pinot-spi`.
## Validation
- `TableNameBuilderTest` (4 tests, 0 failures)
- `spotless:apply -pl pinot-spi`
- `license:format -pl pinot-spi`
- `checkstyle:check -pl pinot-spi`
- `license:check -pl pinot-spi`
- `test-compile -pl pinot-spi -am` with added-line `-Xlint:all` review
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add safe SQL quoting for typed table names [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #19327:
URL: https://github.com/apache/pinot/pull/19327#discussion_r3837362029
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/TableNameBuilder.java:
##########
@@ -117,6 +118,47 @@ public static boolean isRealtimeTableResource(String resourceName) {
return REALTIME.tableHasTypeSuffix(resourceName);
}
+ /// Quotes a typed table name for use as a SQL identifier. When the table name is database-qualified, the database
+ /// and table components are quoted separately. Embedded double quotes are escaped by doubling them.
+ ///
+ /// @param tableNameWithType Table name ending in `_OFFLINE` or `_REALTIME`, optionally prefixed with a database name
+ /// @return Table name quoted for use in a SQL statement
+ /// @throws IllegalArgumentException If the table name is not a valid typed table resource
+ public static String quoteTableNameWithType(String tableNameWithType) {
+ if (tableNameWithType == null || containsWhitespace(tableNameWithType)) {
+ throw new IllegalArgumentException("Invalid table name with type");
+ }
+
+ int separatorIndex = tableNameWithType.indexOf('.');
+ if (separatorIndex < 0) {
+ validateTableResourceName(tableNameWithType);
+ return StringUtil.quoteSqlIdentifier(tableNameWithType);
+ }
+ if (separatorIndex == 0 || separatorIndex != tableNameWithType.lastIndexOf('.')) {
+ throw new IllegalArgumentException("Invalid table name with type");
+ }
+
+ String tableResourceName = tableNameWithType.substring(separatorIndex + 1);
+ validateTableResourceName(tableResourceName);
+ return StringUtil.quoteSqlIdentifier(tableNameWithType.substring(0, separatorIndex)) + "."
+ + StringUtil.quoteSqlIdentifier(tableResourceName);
+ }
+
+ private static void validateTableResourceName(String tableResourceName) {
Review Comment:
`tableResourceName` is the same as `tableNameWithType`. Suggest making the names consistent
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/StringUtil.java:
##########
@@ -36,6 +36,15 @@ public static String join(String separator, String... keys) {
return StringUtils.join(keys, separator);
}
+ /// Quotes one SQL identifier component with double quotes, escaping embedded double quotes by doubling them.
+ /// Qualified names must be split by the caller so that each component is quoted separately.
+ ///
+ /// @param identifier SQL identifier component
+ /// @return Identifier quoted for use in a SQL statement
+ public static String quoteSqlIdentifier(String identifier) {
Review Comment:
I don't think this belongs to this util. Please create a sql util if none exists
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add safe SQL quoting for typed table names [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #19327:
URL: https://github.com/apache/pinot/pull/19327
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add safe SQL quoting for typed table names [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #19327:
URL: https://github.com/apache/pinot/pull/19327#discussion_r3837427556
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/TableNameBuilder.java:
##########
@@ -117,6 +118,47 @@ public static boolean isRealtimeTableResource(String resourceName) {
return REALTIME.tableHasTypeSuffix(resourceName);
}
+ /// Quotes a typed table name for use as a SQL identifier. When the table name is database-qualified, the database
+ /// and table components are quoted separately. Embedded double quotes are escaped by doubling them.
+ ///
+ /// @param tableNameWithType Table name ending in `_OFFLINE` or `_REALTIME`, optionally prefixed with a database name
+ /// @return Table name quoted for use in a SQL statement
+ /// @throws IllegalArgumentException If the table name is not a valid typed table resource
+ public static String quoteTableNameWithType(String tableNameWithType) {
+ if (tableNameWithType == null || containsWhitespace(tableNameWithType)) {
+ throw new IllegalArgumentException("Invalid table name with type");
+ }
+
+ int separatorIndex = tableNameWithType.indexOf('.');
+ if (separatorIndex < 0) {
+ validateTableResourceName(tableNameWithType);
+ return StringUtil.quoteSqlIdentifier(tableNameWithType);
+ }
+ if (separatorIndex == 0 || separatorIndex != tableNameWithType.lastIndexOf('.')) {
+ throw new IllegalArgumentException("Invalid table name with type");
+ }
+
+ String tableResourceName = tableNameWithType.substring(separatorIndex + 1);
+ validateTableResourceName(tableResourceName);
+ return StringUtil.quoteSqlIdentifier(tableNameWithType.substring(0, separatorIndex)) + "."
+ + StringUtil.quoteSqlIdentifier(tableResourceName);
+ }
+
+ private static void validateTableResourceName(String tableResourceName) {
Review Comment:
Renamed the validation helper and local variable to use tableNameWithType terminology consistently.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/StringUtil.java:
##########
@@ -36,6 +36,15 @@ public static String join(String separator, String... keys) {
return StringUtils.join(keys, separator);
}
+ /// Quotes one SQL identifier component with double quotes, escaping embedded double quotes by doubling them.
+ /// Qualified names must be split by the caller so that each component is quoted separately.
+ ///
+ /// @param identifier SQL identifier component
+ /// @return Identifier quoted for use in a SQL statement
+ public static String quoteSqlIdentifier(String identifier) {
Review Comment:
Moved identifier quoting into the new SqlUtils.quoteIdentifier() utility and migrated all callers and tests.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add safe SQL quoting for typed table names [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19327:
URL: https://github.com/apache/pinot/pull/19327#issuecomment-5365094421
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19327?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `19 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 38.94%. Comparing base ([`f8352be`](https://app.codecov.io/gh/apache/pinot/commit/f8352be252fc1a6cf099480a7812d1555d264e0e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`acf6ef4`](https://app.codecov.io/gh/apache/pinot/commit/acf6ef46601df87df68b3bdf861c0b71fb5b5218?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/19327?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ache/pinot/spi/utils/builder/TableNameBuilder.java](https://app.codecov.io/gh/apache/pinot/pull/19327?src=pr&el=tree&filepath=pinot-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fspi%2Futils%2Fbuilder%2FTableNameBuilder.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvYnVpbGRlci9UYWJsZU5hbWVCdWlsZGVyLmphdmE=) | 0.00% | [19 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19327?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (f8352be) and HEAD (acf6ef4). Click for more details.
>
> <details><summary>HEAD has 24 uploads less than BASE</summary>
>
>| Flag | BASE (f8352be) | HEAD (acf6ef4) |
>|------|------|------|
>|lane-a|2|0|
>|integration|4|0|
>|java-25|6|1|
>|integration2|2|0|
>|temurin|6|1|
>|lane-b|2|0|
>|integration1|2|0|
>|unittests1|1|0|
>|unittests|2|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19327 +/- ##
=============================================
- Coverage 67.00% 38.94% -28.06%
+ Complexity 1424 1417 -7
=============================================
Files 3463 3463
Lines 221671 221690 +19
Branches 34954 34957 +3
=============================================
- Hits 148524 86335 -62189
- Misses 61310 127483 +66173
+ Partials 11837 7872 -3965
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19327/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19327/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19327/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19327/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19327/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.94% <0.00%> (-28.06%)` | :arrow_down: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19327/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19327/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19327/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.94% <0.00%> (-28.06%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19327/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.94% <0.00%> (-28.06%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19327/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19327/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.94% <0.00%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19327?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org