Allow table name with dots by a PinotConfiguration switch #8713

dongxiaoman · 2022-05-16T22:20:08Z

Description

Following changes in #5734,
Allows table name with dots by a shared pinot property. This will allow table name format like namespace.table_name to avoid table_name conflict while providing some mitigation/middle ground before Pinot table goes true multi-tenant.

The use case this one PR enabled will be for some users sharing a same Pinot Cluster, with the below Example rules (not in Pinot, but only with the wrapper/UI or some toolset visible to user):

The Admin enforces the rule that the users cannot use dots in their own table_name like test;
The Admin also enforces the rule that the users will be given a unique "namespace" as prefix to table;
The users can create a table named test and write query freely/naturally with table name like user_namespace.test
Pinot can delegate the authorization of table access via its plugin properly in the query

Extended feature

Extract column name only after the last dot in cases like SELECT db.namespace.table.column_name from db.namespace.table. In the example the table name will be db.namespace.table

Tests Done

Many Unit tests

Release Notes

Added pinot.table.name.dots.enabled config to Controllers and Brokers to allow table name with dots.
Extract column name from queries like "SELECT db.namespace.table.column_name" properly

pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java

pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java

pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java

apucher · 2022-05-17T05:26:39Z

IMO this doesn't even need a feature flag. We're not breaking any behavior. Keeps things simple.

Jackie-Jiang · 2022-05-17T18:21:13Z

@apucher Good point. Let me list down the things we are trying to handle with . and discuss a solution:

SELECT ... FROM <db>.<table>
SELECT <table>.<col> FROM ...
In the future, we also want to support:
SELECT <col>.<nested_fields> FROM ...

If we allow . within the table name, we cannot tell if . is the delimiter or part of the table name.
Add @xiangfu0 to the discussion

apucher · 2022-05-17T18:31:53Z

it should be easy to parse the table name since it needs to be present in the FROM clause.

are nested field selects a thing yet?

Jackie-Jiang · 2022-05-17T21:30:50Z

it should be easy to parse the table name since it needs to be present in the FROM clause.

I think the <db>.<table> convention is from the connector, and we don't have the database concept in Pinot. When getting it from the FROM clause, we want to match it with the tables available in pinot. If we support dot in the table name, it will make it slightly hard to parse the name. E.g. when getting a.b.c.d.e, we won't know which part is the table name.

are nested field selects a thing yet?

We don't support it yet, but it is in our roadmap, so we should take them into consideration

I think using dot in table name (or any field name) is in general bad practice because dot is already reserved for other purpose. @dongxiaoman I think we should just use _ instead of . for multi-tenant purpose. To avoid name conflict, we can reject namespace with underlying _ or perform a prefix check when adding a new namespace.

Some reference about not using .:

apucher · 2022-05-17T22:09:50Z

There's precedent for using dots as separators across databases/schemas in MySQL, MSSQL, and others. It's tricky in the sense that underscores are much more commonly used in name tables. Both, underscores and dots, could work. My hunch is that we'll ultimately have to support dots (to separate namespaces) as most cloud analytics systems already do:

Jackie-Jiang · 2022-05-17T22:27:09Z

Agree on using dot to connect db/namespace and table name and column name (this is the standard SQL syntax).
The problem here is that we don't support db/namespace concept natively in Pinot right now, and we are trying to overload the table name to include the namespace as part of it. This can potentially become a blocker for us when adding support of namespace in the future.

dongxiaoman · 2022-05-18T16:45:45Z

@Jackie-Jiang Agree that using dot in table name is in general practice not so compatible. I already saw JDBC H2 error if I copy/paste the Full integration test because JDBC engine treat dots as separator by default. That is why I hide the support with a feature flag. cc @apucher . The H2 error can be bypassed by using backticks.

Meanwhile, as I mentioned, table name with dots can provide a transition/middle ground before we fully support full multi-tenant/DB concept.
By this PR, we can allow the user write something naturally:
SELECT col1 from mydb.test_table where xxx while the mydb.test_table is managed by Pinot admins, and the user is assigned mydb from the DataWarehouse infrastructure on top of Pinot.

If this is not breaking other features, providing extra flexibility (extra knob) will help support more use cases, it actually expands Pinot's scope

Jackie-Jiang · 2022-05-18T23:01:45Z

Sounds good. I think we can limit the flexibility of the table name to contain at most one dot, and assume the name is constructed from <db>.<table>. There are 2 places where we handle the dots within the table name:

getActualTableName(): With the above limitation, no change is needed. The queried table name should have at most one dot
getActualColumnName(): We can change the logic to do a prefix match for the rawTableName + '.' (use columnName.regionMatches() for better performance)

...t-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java

Jackie-Jiang · 2022-05-19T18:25:08Z

pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java

    String tableName = tableConfig.getTableName();
-    if (tableName.contains(".") || tableName.contains(" ")) {
-      throw new IllegalStateException("Table name: '" + tableName + "' containing '.' or space is not allowed");
+    if (tableName.contains(".") && !allowDots) {


Let's check that the table name contains at most one dot, and put some comments explaining we allow table name with embedded database name

Added logic and also unit tests

pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java

Jackie-Jiang · 2022-05-19T18:38:48Z

pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java

   */
-  private String getActualTableName(String tableName) {
+  @VisibleForTesting
+  static String getActualTableName(String tableName, TableCache tableCache, BrokerRoutingManager routingManager,


We might not want to change this method. Even if we allow table name with database, we should probably still support the case of table name without database.

Right now the code change has only one effect:
If we allow dots in table name, we will avoid striping the [database_name]. from the table name. This will be needed in our case, that we wish to treat [database_name].[table_name] as the whole actual table name.

This behavior is under the feature flag so will be backwards compatible.

What I meant is that the existing code should already be able to handle the [database_name].[table_name] as the table name (line 773, 801, 812 in the original code). We don't need to introduce the extra logic of reading properties from the config.

I see. I did not expect the behavior to be the same but you are right.
I am still keeping the unit tests since they will ensure our expected behavior is not broken

codecov-commenter · 2022-05-19T19:06:47Z

Codecov Report

Merging #8713 (3d1ec09) into master (b6ed914) will decrease coverage by 5.31%.
The diff coverage is 61.02%.

@@             Coverage Diff              @@
##             master    #8713      +/-   ##
============================================
- Coverage     68.17%   62.86%   -5.32%     
+ Complexity     4619     4605      -14     
============================================
  Files          1735     1689      -46     
  Lines         91320    89304    -2016     
  Branches      13644    13415     -229     
============================================
- Hits          62258    56141    -6117     
- Misses        24714    29132    +4418     
+ Partials       4348     4031     -317

Flag	Coverage Δ
integration2	`?`
unittests1	`66.23% <46.66%> (+0.01%)`	⬆️
unittests2	`14.14% <55.88%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...pache/pinot/common/config/provider/TableCache.java	`0.00% <0.00%> (-71.38%)`	⬇️
...va/org/apache/pinot/spi/utils/CommonConstants.java	`22.22% <ø> (ø)`
...ntroller/helix/core/PinotHelixResourceManager.java	`64.08% <56.70%> (-2.77%)`	⬇️
...roker/requesthandler/BaseBrokerRequestHandler.java	`33.02% <84.21%> (-37.41%)`	⬇️
...oller/api/resources/PinotTableRestletResource.java	`46.82% <100.00%> (-5.53%)`	⬇️
...ler/api/resources/TableConfigsRestletResource.java	`75.58% <100.00%> (+0.14%)`	⬆️
...he/pinot/segment/local/utils/TableConfigUtils.java	`67.65% <100.00%> (+0.72%)`	⬆️
...va/org/apache/pinot/core/routing/RoutingTable.java	`0.00% <0.00%> (-100.00%)`	⬇️
...va/org/apache/pinot/common/config/NettyConfig.java	`0.00% <0.00%> (-100.00%)`	⬇️
...a/org/apache/pinot/common/metrics/MinionMeter.java	`0.00% <0.00%> (-100.00%)`	⬇️
... and 358 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b6ed914...3d1ec09. Read the comment docs.

pinot-common/src/main/java/org/apache/pinot/common/config/provider/TableCache.java

Jackie-Jiang

LGTM other wise

pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java

...oller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java

...ler/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java

pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java

dongxiaoman · 2022-05-31T23:23:02Z

@Jackie-Jiang Sorry for the late response. Can you please have another quick look? All comments addressed

Jackie-Jiang reviewed May 16, 2022

View reviewed changes

dongxiaoman force-pushed the xd-table-name-dots branch from f4f77f9 to c484a75 Compare May 19, 2022 18:20

Jackie-Jiang reviewed May 19, 2022

View reviewed changes

dongxiaoman force-pushed the xd-table-name-dots branch from 6fb5f84 to 2b6898a Compare May 24, 2022 00:07

dongxiaoman commented May 24, 2022

View reviewed changes

pinot-common/src/main/java/org/apache/pinot/common/config/provider/TableCache.java Outdated Show resolved Hide resolved

dongxiaoman force-pushed the xd-table-name-dots branch from 80edebc to 9e8d510 Compare May 24, 2022 18:12

dongxiaoman requested a review from Jackie-Jiang May 25, 2022 16:09

Jackie-Jiang approved these changes May 25, 2022

View reviewed changes

dongxiaoman force-pushed the xd-table-name-dots branch 2 times, most recently from cbd751f to 94d6b8c Compare May 31, 2022 17:15

Xiaoman Dong added 11 commits May 31, 2022 13:59

allow dots with config

99a4fd6

fix lint

bffe419

avoid formatter change

b68c5ee

address comments

b1d4f93

address comments

4210f5f

test build failure

195197a

fix failed build

9ea644b

use boolean for validation

b0f5924

checkstyle

80e9e5e

address bug

9bc8db7

integration test

708fafb

Xiaoman Dong added 8 commits May 31, 2022 13:59

tweak code

2720c4a

extra comment

7683fa3

checkstyle

eebc455

remove splitByLastDot()

e602c65

avoid changing one func

c6e5264

save changes from feedback

93aa369

fix comments

aa9d4b0

tune comments

3d1ec09

dongxiaoman force-pushed the xd-table-name-dots branch from 94d6b8c to 3d1ec09 Compare May 31, 2022 20:59

dongxiaoman requested a review from Jackie-Jiang May 31, 2022 23:22

Jackie-Jiang added release-notes Referenced by PRs that need attention when compiling the next release notes feature labels Jun 1, 2022

Jackie-Jiang merged commit ae39243 into apache:master Jun 1, 2022

walterddr mentioned this pull request Feb 6, 2024

[multistage] support database/namespace in v2 #12377

Closed

3 tasks

shounakmk219 mentioned this pull request Feb 12, 2024

Remove the feature flag allow.table.name.with.database #12402

Merged

shounakmk219 mentioned this pull request Mar 7, 2024

[multistage] support database in v2 #12591

Merged

Allow table name with dots by a PinotConfiguration switch #8713

Allow table name with dots by a PinotConfiguration switch #8713

Uh oh!

Conversation

dongxiaoman commented May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Extended feature

Tests Done

Release Notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

apucher commented May 17, 2022

Uh oh!

Jackie-Jiang commented May 17, 2022

Uh oh!

apucher commented May 17, 2022

Uh oh!

Jackie-Jiang commented May 17, 2022

Uh oh!

apucher commented May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jackie-Jiang commented May 17, 2022

Uh oh!

dongxiaoman commented May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jackie-Jiang commented May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Jackie-Jiang May 19, 2022

Choose a reason for hiding this comment

Uh oh!

dongxiaoman May 24, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jackie-Jiang May 19, 2022

Choose a reason for hiding this comment

Uh oh!

dongxiaoman May 24, 2022

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang May 24, 2022

Choose a reason for hiding this comment

Uh oh!

dongxiaoman May 25, 2022

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dongxiaoman commented May 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dongxiaoman commented May 16, 2022 •

edited

Loading

apucher commented May 17, 2022 •

edited

Loading

dongxiaoman commented May 18, 2022 •

edited

Loading

Jackie-Jiang commented May 18, 2022 •

edited

Loading

codecov-commenter commented May 19, 2022 •

edited

Loading