This is an automated email from the ASF dual-hosted git repository.
zabetak pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/calcite.git
The following commit(s) were added to refs/heads/master by this push:
new 48126d3 [CALCITE-4706] JDBC adapter generates casts exceeding
Redshift's data types bounds
48126d3 is described below
commit 48126d3d3834e28e84ae3fc4c65b86831b5d6aef
Author: Stamatis Zampetakis <zabe...@gmail.com>
AuthorDate: Thu Jul 29 14:16:20 2021 +0300
[CALCITE-4706] JDBC adapter generates casts exceeding Redshift's data types
bounds
1. Add Redshift type system ensuring precision/scale bounds are
satisfied when casting.
2. Consider max precision for DECIMAL and CHAR data types in
SqlDialect#getCastSpec.
3. Consider max scale for DECIMAL data types in SqlDialect#getCastSpec.
4. Extend RelToSqlConverterTest.Sql with custom type system to allow
writting tests with non-default type system (e.g., simulate systems with
large numeric precision).
5. Set typesystem explicitly in SqlDialectFactoryImpl since it cannot
be derived from the metadata and the default is not appropriate.
6. Add unit tests casting to DECIMAL, CHAR, VARCHAR, with
precision/scale exceeding Redshift's bounds.
Close apache/calcite#2470
---
.../java/org/apache/calcite/sql/SqlDialect.java | 7 +-
.../apache/calcite/sql/SqlDialectFactoryImpl.java | 3 +-
.../calcite/sql/dialect/RedshiftSqlDialect.java | 28 ++++++-
.../org/apache/calcite/sql/type/SqlTypeUtil.java | 8 +-
.../rel/rel2sql/RelToSqlConverterStructsTest.java | 3 +-
.../calcite/rel/rel2sql/RelToSqlConverterTest.java | 97 ++++++++++++++++++----
6 files changed, 124 insertions(+), 22 deletions(-)
diff --git a/core/src/main/java/org/apache/calcite/sql/SqlDialect.java
b/core/src/main/java/org/apache/calcite/sql/SqlDialect.java
index 0942973..06d5de9 100644
--- a/core/src/main/java/org/apache/calcite/sql/SqlDialect.java
+++ b/core/src/main/java/org/apache/calcite/sql/SqlDialect.java
@@ -789,10 +789,15 @@ public class SqlDialect {
* {@code CAST(NULL AS <nulltype>)} is rendered as {@code NULL}. */
public @Nullable SqlNode getCastSpec(RelDataType type) {
int maxPrecision = -1;
+ int maxScale = -1;
if (type instanceof AbstractSqlType) {
switch (type.getSqlTypeName()) {
case NULL:
return null;
+ case DECIMAL:
+ maxScale = getTypeSystem().getMaxScale(type.getSqlTypeName());
+ // fall through
+ case CHAR:
case VARCHAR:
// if needed, adjust varchar length to max length supported by the
system
maxPrecision = getTypeSystem().getMaxPrecision(type.getSqlTypeName());
@@ -803,7 +808,7 @@ public class SqlDialect {
String charSet = type.getCharset() != null && supportsCharSet()
? type.getCharset().name()
: null;
- return SqlTypeUtil.convertTypeToSpec(type, charSet, maxPrecision);
+ return SqlTypeUtil.convertTypeToSpec(type, charSet, maxPrecision,
maxScale);
}
return SqlTypeUtil.convertTypeToSpec(type);
}
diff --git
a/core/src/main/java/org/apache/calcite/sql/SqlDialectFactoryImpl.java
b/core/src/main/java/org/apache/calcite/sql/SqlDialectFactoryImpl.java
index 91f0f31..b7d025a 100644
--- a/core/src/main/java/org/apache/calcite/sql/SqlDialectFactoryImpl.java
+++ b/core/src/main/java/org/apache/calcite/sql/SqlDialectFactoryImpl.java
@@ -129,7 +129,8 @@ public class SqlDialectFactoryImpl implements
SqlDialectFactory {
return new MysqlSqlDialect(
c.withDataTypeSystem(MysqlSqlDialect.MYSQL_TYPE_SYSTEM));
case "REDSHIFT":
- return new RedshiftSqlDialect(c);
+ return new RedshiftSqlDialect(
+ c.withDataTypeSystem(RedshiftSqlDialect.TYPE_SYSTEM));
case "SNOWFLAKE":
return new SnowflakeSqlDialect(c);
case "SPARK":
diff --git
a/core/src/main/java/org/apache/calcite/sql/dialect/RedshiftSqlDialect.java
b/core/src/main/java/org/apache/calcite/sql/dialect/RedshiftSqlDialect.java
index 5785520..0d498de 100644
--- a/core/src/main/java/org/apache/calcite/sql/dialect/RedshiftSqlDialect.java
+++ b/core/src/main/java/org/apache/calcite/sql/dialect/RedshiftSqlDialect.java
@@ -18,12 +18,15 @@ package org.apache.calcite.sql.dialect;
import org.apache.calcite.avatica.util.Casing;
import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeSystem;
+import org.apache.calcite.rel.type.RelDataTypeSystemImpl;
import org.apache.calcite.sql.SqlDataTypeSpec;
import org.apache.calcite.sql.SqlDialect;
import org.apache.calcite.sql.SqlNode;
import org.apache.calcite.sql.SqlUserDefinedTypeNameSpec;
import org.apache.calcite.sql.SqlWriter;
import org.apache.calcite.sql.parser.SqlParserPos;
+import org.apache.calcite.sql.type.SqlTypeName;
import org.checkerframework.checker.nullness.qual.Nullable;
@@ -31,12 +34,35 @@ import org.checkerframework.checker.nullness.qual.Nullable;
* A <code>SqlDialect</code> implementation for the Redshift database.
*/
public class RedshiftSqlDialect extends SqlDialect {
+ public static final RelDataTypeSystem TYPE_SYSTEM =
+ new RelDataTypeSystemImpl() {
+ @Override public int getMaxPrecision(SqlTypeName typeName) {
+ switch (typeName) {
+ case VARCHAR:
+ return 65535;
+ case CHAR:
+ return 4096;
+ default:
+ return super.getMaxPrecision(typeName);
+ }
+ }
+
+ @Override public int getMaxNumericPrecision() {
+ return 38;
+ }
+
+ @Override public int getMaxNumericScale() {
+ return 37;
+ }
+ };
+
public static final SqlDialect.Context DEFAULT_CONTEXT =
SqlDialect.EMPTY_CONTEXT
.withDatabaseProduct(SqlDialect.DatabaseProduct.REDSHIFT)
.withIdentifierQuoteString("\"")
.withQuotedCasing(Casing.TO_LOWER)
.withUnquotedCasing(Casing.TO_LOWER)
- .withCaseSensitive(false);
+ .withCaseSensitive(false)
+ .withDataTypeSystem(TYPE_SYSTEM);
public static final SqlDialect DEFAULT = new
RedshiftSqlDialect(DEFAULT_CONTEXT);
diff --git a/core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java
b/core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java
index 714371b..3fdd416 100644
--- a/core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java
+++ b/core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java
@@ -1021,10 +1021,11 @@ public abstract class SqlTypeUtil {
* @param type type descriptor
* @param charSetName charSet name
* @param maxPrecision The max allowed precision.
+ * @param maxScale max allowed scale
* @return corresponding parse representation
*/
public static SqlDataTypeSpec convertTypeToSpec(RelDataType type,
- @Nullable String charSetName, int maxPrecision) {
+ @Nullable String charSetName, int maxPrecision, int maxScale) {
SqlTypeName typeName = type.getSqlTypeName();
// TODO jvs 28-Dec-2004: support row types, user-defined types,
@@ -1039,6 +1040,9 @@ public abstract class SqlTypeUtil {
precision = maxPrecision;
}
int scale = typeName.allowsScale() ? type.getScale() : -1;
+ if (maxScale > 0 && scale > maxScale) {
+ scale = maxScale;
+ }
typeNameSpec = new SqlBasicTypeNameSpec(
typeName,
@@ -1084,7 +1088,7 @@ public abstract class SqlTypeUtil {
public static SqlDataTypeSpec convertTypeToSpec(RelDataType type) {
// TODO jvs 28-Dec-2004: collation
String charSetName = inCharFamily(type) ? type.getCharset().name() : null;
- return convertTypeToSpec(type, charSetName, -1);
+ return convertTypeToSpec(type, charSetName, -1, -1);
}
public static RelDataType createMultisetType(
diff --git
a/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterStructsTest.java
b/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterStructsTest.java
index 284bc10..e92f102 100644
---
a/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterStructsTest.java
+++
b/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterStructsTest.java
@@ -16,6 +16,7 @@
*/
package org.apache.calcite.rel.rel2sql;
+import org.apache.calcite.rel.type.RelDataTypeSystem;
import org.apache.calcite.sql.dialect.CalciteSqlDialect;
import org.apache.calcite.sql.parser.SqlParser;
import org.apache.calcite.test.CalciteAssert;
@@ -36,7 +37,7 @@ class RelToSqlConverterStructsTest {
private RelToSqlConverterTest.Sql sql(String sql) {
return new RelToSqlConverterTest.Sql(CalciteAssert.SchemaSpec.MY_DB, sql,
CalciteSqlDialect.DEFAULT, SqlParser.Config.DEFAULT, ImmutableSet.of(),
- UnaryOperator.identity(), null, ImmutableList.of());
+ UnaryOperator.identity(), null, ImmutableList.of(),
RelDataTypeSystem.DEFAULT);
}
@Test void testNestedSchemaSelectStar() {
diff --git
a/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
b/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
index 51ed1f3..94ef0c0 100644
---
a/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
+++
b/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
@@ -91,6 +91,7 @@ import org.junit.jupiter.api.Test;
import java.util.Collection;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.Set;
import java.util.function.Consumer;
import java.util.function.Function;
@@ -115,7 +116,7 @@ class RelToSqlConverterTest {
private Sql sql(String sql) {
return new Sql(CalciteAssert.SchemaSpec.JDBC_FOODMART, sql,
CalciteSqlDialect.DEFAULT, SqlParser.Config.DEFAULT, ImmutableSet.of(),
- UnaryOperator.identity(), null, ImmutableList.of());
+ UnaryOperator.identity(), null, ImmutableList.of(),
RelDataTypeSystem.DEFAULT);
}
/** Initiates a test case with a given {@link RelNode} supplier. */
@@ -128,7 +129,7 @@ class RelToSqlConverterTest {
private static Planner getPlanner(List<RelTraitDef> traitDefs,
SqlParser.Config parserConfig, SchemaPlus schema,
SqlToRelConverter.Config sqlToRelConf, Collection<SqlLibrary> librarySet,
- Program... programs) {
+ RelDataTypeSystem typeSystem, Program... programs) {
final MockSqlOperatorTable operatorTable =
new MockSqlOperatorTable(
SqlOperatorTables.chain(SqlStdOperatorTable.instance(),
@@ -142,6 +143,7 @@ class RelToSqlConverterTest {
.sqlToRelConverterConfig(sqlToRelConf)
.programs(programs)
.operatorTable(operatorTable)
+ .typeSystem(typeSystem)
.build();
return Frameworks.getPlanner(config);
}
@@ -711,6 +713,59 @@ class RelToSqlConverterTest {
sql(query).ok(expected);
}
+ /**
+ * Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-4706";>[CALCITE-4706]
+ * JDBC adapter generates casts exceeding Redshift's data types bounds</a>.
+ */
+ @Test void testCastDecimalBigPrecision() {
+ final String query = "select cast(\"product_id\" as decimal(60,2)) "
+ + "from \"product\" ";
+ final String expectedRedshift = "SELECT CAST(\"product_id\" AS DECIMAL(38,
2))\n"
+ + "FROM \"foodmart\".\"product\"";
+ sql(query)
+ .withTypeSystem(new RelDataTypeSystemImpl() {
+ @Override public int getMaxNumericPrecision() {
+ // Ensures that parsed decimal will not be truncated during SQL to
Rel transformation
+ // The default type system sets precision to 19 so it is not
sufficient to test
+ // this change.
+ return 100;
+ }
+ })
+ .withRedshift()
+ .ok(expectedRedshift);
+ }
+
+ /**
+ * Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-4706";>[CALCITE-4706]
+ * JDBC adapter generates casts exceeding Redshift's data types bounds</a>.
+ */
+ @Test void testCastDecimalBigScale() {
+ final String query = "select cast(\"product_id\" as decimal(2,90)) "
+ + "from \"product\" ";
+ final String expectedRedshift = "SELECT CAST(\"product_id\" AS DECIMAL(2,
37))\n"
+ + "FROM \"foodmart\".\"product\"";
+ sql(query)
+ .withRedshift()
+ .ok(expectedRedshift);
+ }
+
+ /**
+ * Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-4706";>[CALCITE-4706]
+ * JDBC adapter generates casts exceeding Redshift's data types bounds</a>.
+ */
+ @Test void testCastLongChar() {
+ final String query = "select cast(\"product_id\" as char(9999999)) "
+ + "from \"product\" ";
+ final String expectedRedshift = "SELECT CAST(\"product_id\" AS
CHAR(4096))\n"
+ + "FROM \"foodmart\".\"product\"";
+ sql(query)
+ .withRedshift()
+ .ok(expectedRedshift);
+ }
+
/** Test case for
* <a
href="https://issues.apache.org/jira/browse/CALCITE-2713";>[CALCITE-2713]
* JDBC adapter may generate casts on PostgreSQL for VARCHAR type exceeding
@@ -720,15 +775,17 @@ class RelToSqlConverterTest {
+ " from \"expense_fact\"";
final String expectedPostgreSQL = "SELECT CAST(\"store_id\" AS
VARCHAR(256))\n"
+ "FROM \"foodmart\".\"expense_fact\"";
- sql(query)
- .withPostgresqlModifiedTypeSystem()
- .ok(expectedPostgreSQL);
-
final String expectedOracle = "SELECT CAST(\"store_id\" AS VARCHAR(512))\n"
+ "FROM \"foodmart\".\"expense_fact\"";
+ final String expectedRedshift = "SELECT CAST(\"store_id\" AS
VARCHAR(65535))\n"
+ + "FROM \"foodmart\".\"expense_fact\"";
sql(query)
+ .withPostgresqlModifiedTypeSystem()
+ .ok(expectedPostgreSQL)
.withOracleModifiedTypeSystem()
- .ok(expectedOracle);
+ .ok(expectedOracle)
+ .withRedshift()
+ .ok(expectedRedshift);
}
/** Test case for
@@ -5996,12 +6053,14 @@ class RelToSqlConverterTest {
private final List<Function<RelNode, RelNode>> transforms;
private final SqlParser.Config parserConfig;
private final UnaryOperator<SqlToRelConverter.Config> config;
+ private final RelDataTypeSystem typeSystem;
Sql(CalciteAssert.SchemaSpec schemaSpec, String sql, SqlDialect dialect,
SqlParser.Config parserConfig, Set<SqlLibrary> librarySet,
UnaryOperator<SqlToRelConverter.Config> config,
@Nullable Function<RelBuilder, RelNode> relFn,
- List<Function<RelNode, RelNode>> transforms) {
+ List<Function<RelNode, RelNode>> transforms,
+ RelDataTypeSystem typeSystem) {
this.schemaSpec = schemaSpec;
this.sql = sql;
this.dialect = dialect;
@@ -6010,16 +6069,17 @@ class RelToSqlConverterTest {
this.transforms = ImmutableList.copyOf(transforms);
this.parserConfig = parserConfig;
this.config = config;
+ this.typeSystem = Objects.requireNonNull(typeSystem, "typeSystem");
}
Sql dialect(SqlDialect dialect) {
return new Sql(schemaSpec, sql, dialect, parserConfig, librarySet,
config,
- relFn, transforms);
+ relFn, transforms, typeSystem);
}
Sql relFn(Function<RelBuilder, RelNode> relFn) {
return new Sql(schemaSpec, sql, dialect, parserConfig, librarySet,
config,
- relFn, transforms);
+ relFn, transforms, typeSystem);
}
Sql withCalcite() {
@@ -6146,12 +6206,17 @@ class RelToSqlConverterTest {
Sql parserConfig(SqlParser.Config parserConfig) {
return new Sql(schemaSpec, sql, dialect, parserConfig, librarySet,
config,
- relFn, transforms);
+ relFn, transforms, typeSystem);
}
Sql withConfig(UnaryOperator<SqlToRelConverter.Config> config) {
return new Sql(schemaSpec, sql, dialect, parserConfig, librarySet,
config,
- relFn, transforms);
+ relFn, transforms, typeSystem);
+ }
+
+ Sql withTypeSystem(RelDataTypeSystem typeSystem) {
+ return new Sql(schemaSpec, sql, dialect, parserConfig, librarySet,
config,
+ relFn, transforms, typeSystem);
}
final Sql withLibrary(SqlLibrary library) {
@@ -6160,7 +6225,7 @@ class RelToSqlConverterTest {
Sql withLibrarySet(Iterable<? extends SqlLibrary> librarySet) {
return new Sql(schemaSpec, sql, dialect, parserConfig,
- ImmutableSet.copyOf(librarySet), config, relFn, transforms);
+ ImmutableSet.copyOf(librarySet), config, relFn, transforms,
typeSystem);
}
Sql optimize(final RuleSet ruleSet,
@@ -6177,7 +6242,7 @@ class RelToSqlConverterTest {
ImmutableList.of(), ImmutableList.of());
});
return new Sql(schemaSpec, sql, dialect, parserConfig, librarySet,
config,
- relFn, transforms);
+ relFn, transforms, typeSystem);
}
Sql ok(String expectedQuery) {
@@ -6212,7 +6277,7 @@ class RelToSqlConverterTest {
final SqlToRelConverter.Config config =
this.config.apply(SqlToRelConverter.config()
.withTrimUnusedFields(false));
final Planner planner =
- getPlanner(null, parserConfig, defaultSchema, config,
librarySet);
+ getPlanner(null, parserConfig, defaultSchema, config,
librarySet, typeSystem);
SqlNode parse = planner.parse(sql);
SqlNode validate = planner.validate(parse);
rel = planner.rel(validate).rel;
@@ -6228,7 +6293,7 @@ class RelToSqlConverterTest {
public Sql schema(CalciteAssert.SchemaSpec schemaSpec) {
return new Sql(schemaSpec, sql, dialect, parserConfig, librarySet,
config,
- relFn, transforms);
+ relFn, transforms, typeSystem);
}
}
}
