You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by ja...@apache.org on 2024/02/13 05:49:43 UTC
(pinot) branch master updated: Remove the feature flag `allow.table.name.with.database` (#12402)
This is an automated email from the ASF dual-hosted git repository.
jackie pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 04b279e8da Remove the feature flag `allow.table.name.with.database` (#12402)
04b279e8da is described below
commit 04b279e8dae03d46a01ed1587ae335813bcc176f
Author: Shounak kulkarni <sh...@gmail.com>
AuthorDate: Tue Feb 13 11:19:36 2024 +0530
Remove the feature flag `allow.table.name.with.database` (#12402)
---
.../BaseBrokerRequestHandlerTest.java | 41 ----------------------
.../api/resources/PinotTableRestletResource.java | 7 +---
.../api/resources/TableConfigsRestletResource.java | 8 ++---
.../segment/local/utils/TableConfigUtils.java | 13 +++----
.../segment/local/utils/TableConfigUtilsTest.java | 24 ++++---------
.../apache/pinot/spi/utils/CommonConstants.java | 2 --
6 files changed, 14 insertions(+), 81 deletions(-)
diff --git a/pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandlerTest.java b/pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandlerTest.java
index 29d2c5b428..b523fbcd0e 100644
--- a/pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandlerTest.java
+++ b/pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandlerTest.java
@@ -48,7 +48,6 @@ import org.apache.pinot.spi.exception.BadQueryRequestException;
import org.apache.pinot.spi.metrics.PinotMetricUtils;
import org.apache.pinot.spi.trace.RequestContext;
import org.apache.pinot.spi.trace.Tracing;
-import org.apache.pinot.spi.utils.CommonConstants;
import org.apache.pinot.spi.utils.JsonUtils;
import org.apache.pinot.sql.parsers.CalciteSqlParser;
import org.apache.pinot.util.TestUtils;
@@ -144,46 +143,6 @@ public class BaseBrokerRequestHandlerTest {
Assert.assertEquals(wrongColumnName3, "mytable");
}
- @Test
- public void testGetActualTableNameBanningDots() {
- // not allowing dots
- PinotConfiguration configuration = new PinotConfiguration();
- configuration.setProperty(CommonConstants.Helix.ALLOW_TABLE_NAME_WITH_DATABASE, false);
-
- TableCache tableCache = Mockito.mock(TableCache.class);
- when(tableCache.isIgnoreCase()).thenReturn(true);
- when(tableCache.getActualTableName("mytable")).thenReturn("mytable");
- Assert.assertEquals(BaseBrokerRequestHandler.getActualTableName("mytable", tableCache), "mytable");
- when(tableCache.getActualTableName("db.mytable")).thenReturn(null);
- Assert.assertEquals(BaseBrokerRequestHandler.getActualTableName("db.mytable", tableCache), "mytable");
-
- when(tableCache.isIgnoreCase()).thenReturn(false);
- Assert.assertEquals(BaseBrokerRequestHandler.getActualTableName("db.mytable", tableCache), "mytable");
- }
-
- @Test
- public void testGetActualTableNameAllowingDots() {
-
- TableCache tableCache = Mockito.mock(TableCache.class);
- when(tableCache.isIgnoreCase()).thenReturn(true);
- // the tableCache should have only "db.mytable" in it since this is the only table
- when(tableCache.getActualTableName("mytable")).thenReturn(null);
- when(tableCache.getActualTableName("db.mytable")).thenReturn("db.mytable");
- when(tableCache.getActualTableName("other.mytable")).thenReturn(null);
- when(tableCache.getActualTableName("test_table")).thenReturn(null);
-
- Assert.assertEquals(BaseBrokerRequestHandler.getActualTableName("test_table", tableCache), "test_table");
- Assert.assertEquals(BaseBrokerRequestHandler.getActualTableName("mytable", tableCache), "mytable");
-
- Assert.assertEquals(BaseBrokerRequestHandler.getActualTableName("db.mytable", tableCache), "db.mytable");
- Assert.assertEquals(BaseBrokerRequestHandler.getActualTableName("other.mytable", tableCache), "other.mytable");
-
- when(tableCache.isIgnoreCase()).thenReturn(false);
- Assert.assertEquals(BaseBrokerRequestHandler.getActualTableName("db.mytable", tableCache), "db.mytable");
- Assert.assertEquals(BaseBrokerRequestHandler.getActualTableName("db.namespace.mytable", tableCache),
- "db.namespace.mytable");
- }
-
@Test
public void testCancelQuery()
throws Exception {
diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
index c0f11a412b..26fb98ad50 100644
--- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
+++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java
@@ -215,12 +215,7 @@ public class PinotTableRestletResource {
// TableConfigUtils.validate(...) is used across table create/update.
TableConfigUtils.validate(tableConfig, schema, typesToSkip, _controllerConf.isDisableIngestionGroovy());
- // TableConfigUtils.validateTableName(...) checks table name rules.
- // So it won't affect already created tables.
- boolean allowTableNameWithDatabase =
- _controllerConf.getProperty(CommonConstants.Helix.ALLOW_TABLE_NAME_WITH_DATABASE,
- CommonConstants.Helix.DEFAULT_ALLOW_TABLE_NAME_WITH_DATABASE);
- TableConfigUtils.validateTableName(tableConfig, allowTableNameWithDatabase);
+ TableConfigUtils.validateTableName(tableConfig);
} catch (Exception e) {
throw new ControllerApplicationException(LOGGER, e.getMessage(), Response.Status.BAD_REQUEST, e);
}
diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java
index ec3d837e3f..d41373f07f 100644
--- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java
+++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java
@@ -70,7 +70,6 @@ import org.apache.pinot.segment.local.utils.TableConfigUtils;
import org.apache.pinot.spi.config.TableConfigs;
import org.apache.pinot.spi.config.table.TableConfig;
import org.apache.pinot.spi.data.Schema;
-import org.apache.pinot.spi.utils.CommonConstants;
import org.apache.pinot.spi.utils.JsonUtils;
import org.apache.pinot.spi.utils.builder.TableNameBuilder;
import org.glassfish.grizzly.http.server.Request;
@@ -463,21 +462,18 @@ public class TableConfigsRestletResource {
Preconditions.checkState(rawTableName.equals(schema.getSchemaName()),
"'tableName': %s must be equal to 'schemaName' from 'schema': %s", rawTableName, schema.getSchemaName());
SchemaUtils.validate(schema);
- boolean allowTableNameWithDatabase = _controllerConf.getProperty(
- CommonConstants.Helix.ALLOW_TABLE_NAME_WITH_DATABASE,
- CommonConstants.Helix.DEFAULT_ALLOW_TABLE_NAME_WITH_DATABASE);
if (offlineTableConfig != null) {
String offlineRawTableName = TableNameBuilder.extractRawTableName(offlineTableConfig.getTableName());
Preconditions.checkState(offlineRawTableName.equals(rawTableName),
"Name in 'offline' table config: %s must be equal to 'tableName': %s", offlineRawTableName, rawTableName);
- TableConfigUtils.validateTableName(offlineTableConfig, allowTableNameWithDatabase);
+ TableConfigUtils.validateTableName(offlineTableConfig);
TableConfigUtils.validate(offlineTableConfig, schema, typesToSkip, _controllerConf.isDisableIngestionGroovy());
}
if (realtimeTableConfig != null) {
String realtimeRawTableName = TableNameBuilder.extractRawTableName(realtimeTableConfig.getTableName());
Preconditions.checkState(realtimeRawTableName.equals(rawTableName),
"Name in 'realtime' table config: %s must be equal to 'tableName': %s", realtimeRawTableName, rawTableName);
- TableConfigUtils.validateTableName(realtimeTableConfig, allowTableNameWithDatabase);
+ TableConfigUtils.validateTableName(realtimeTableConfig);
TableConfigUtils.validate(realtimeTableConfig, schema, typesToSkip, _controllerConf.isDisableIngestionGroovy());
}
if (offlineTableConfig != null && realtimeTableConfig != null) {
diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
index 99ca28263c..45889095b2 100644
--- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
+++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
@@ -195,21 +195,16 @@ public final class TableConfigUtils {
/**
* Validates the table name with the following rules:
* <ul>
- * <li>If there is a flag allowing database name in it, table name can have one dot in it.
- * <li>Otherwise, there is no dot allowed in table name.</li>
+ * <li>Table name can have at most one dot in it.
+ * <li>Table name does not have whitespace.</li>
* </ul>
*/
- public static void validateTableName(TableConfig tableConfig, boolean allowTableNameWithDatabase) {
+ public static void validateTableName(TableConfig tableConfig) {
String tableName = tableConfig.getTableName();
int dotCount = StringUtils.countMatches(tableName, '.');
- // For transitioning into full [database_name].[table_name] support, we allow the table name
- // with one dot at max, so the admin may create mydb.mytable with a feature knob.
- if (allowTableNameWithDatabase && dotCount > 1) {
+ if (dotCount > 1) {
throw new IllegalStateException("Table name: '" + tableName + "' containing more than one '.' is not allowed");
}
- if (!allowTableNameWithDatabase && dotCount > 0) {
- throw new IllegalStateException("Table name: '" + tableName + "' containing '.' is not allowed");
- }
if (StringUtils.containsWhitespace(tableName)) {
throw new IllegalStateException("Table name: '" + tableName + "' containing space is not allowed");
}
diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java
index 1c9b52c4cf..7c18185cae 100644
--- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java
+++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java
@@ -961,33 +961,23 @@ public class TableConfigUtilsTest {
@Test
public void testTableName() {
- String[] malformedTableName = {"test.table", "test table"};
+ String[] malformedTableName = {"test.test.table", "test table"};
for (int i = 0; i < 2; i++) {
String tableName = malformedTableName[i];
TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(tableName).build();
try {
- TableConfigUtils.validateTableName(tableConfig, CommonConstants.Helix.DEFAULT_ALLOW_TABLE_NAME_WITH_DATABASE);
+ TableConfigUtils.validateTableName(tableConfig);
Assert.fail("Should fail for malformed table name : " + tableName);
} catch (IllegalStateException e) {
// expected
}
}
- String allowedWithConfig = "test.table";
- TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(allowedWithConfig).build();
- try {
- TableConfigUtils.validateTableName(tableConfig, true);
- } catch (IllegalStateException e) {
- Assert.fail("Should allow table name with dot if configuration is turned on");
- }
-
- String rejected = "test.another.table";
- tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(rejected).build();
- try {
- TableConfigUtils.validateTableName(tableConfig, true);
- Assert.fail("Should fail for malformed table name : " + rejected);
- } catch (IllegalStateException e) {
- // expected
+ String[] allowedTableName = {"test.table", "testTable"};
+ for (int i = 0; i < 2; i++) {
+ String tableName = allowedTableName[i];
+ TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(tableName).build();
+ TableConfigUtils.validateTableName(tableConfig);
}
}
diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java b/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java
index 4a39e278ab..6a0831ad1d 100644
--- a/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java
+++ b/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java
@@ -91,8 +91,6 @@ public class CommonConstants {
public static final String ENABLE_CASE_INSENSITIVE_KEY = "enable.case.insensitive";
public static final boolean DEFAULT_ENABLE_CASE_INSENSITIVE = true;
- public static final String ALLOW_TABLE_NAME_WITH_DATABASE = "allow.table.name.with.database";
- public static final boolean DEFAULT_ALLOW_TABLE_NAME_WITH_DATABASE = false;
public static final String DEFAULT_HYPERLOGLOG_LOG2M_KEY = "default.hyperloglog.log2m";
public static final int DEFAULT_HYPERLOGLOG_LOG2M = 8;
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org