jacques-n commented on a change in pull request #2572:
URL: https://github.com/apache/calcite/pull/2572#discussion_r744302143
##########
File path: file/src/main/java/org/apache/calcite/adapter/file/CsvEnumerator.java
##########
@@ -130,30 +129,108 @@ public static RelDataType deduceRowType(JavaTypeFactory
typeFactory,
}
for (String string : strings) {
final String name;
- final CsvFieldType fieldType;
+ final RelDataType fieldType;
final int colon = string.indexOf(':');
if (colon >= 0) {
name = string.substring(0, colon);
String typeString = string.substring(colon + 1);
- fieldType = CsvFieldType.of(typeString);
- if (fieldType == null) {
- System.out.println("WARNING: Found unknown type: "
- + typeString + " in file: " + source.path()
- + " for column: " + name
- + ". Will assume the type of column is string");
+ Matcher decimalMatcher = DECIMAL_TYPE_PATTERN.matcher(typeString);
+ if (decimalMatcher.matches()) {
+ int precision = Integer.parseInt(decimalMatcher.group(1));
+ int scale = Integer.parseInt(decimalMatcher.group(2));
+ fieldType = typeFactory.createTypeWithNullability(typeFactory
+ .createSqlType(SqlTypeName.DECIMAL,
+ precision, scale), true);
+ } else {
+ switch (typeString) {
+ case "string":
+ fieldType = typeFactory.createTypeWithNullability(typeFactory
+
.createSqlType(typeFactory.createJavaType(String.class).getSqlTypeName()),
+ true);
+ break;
+ case "boolean":
+ fieldType = typeFactory.createTypeWithNullability(
Review comment:
This pattern seems very redundant. It seems like 99% of the time, the
pattern is string => sqlTypeName (everything except decimal). Can your refactor
to avoid all the repetition?
##########
File path: file/src/main/java/org/apache/calcite/adapter/file/CsvEnumerator.java
##########
@@ -284,6 +361,17 @@ static CSVReader openCsv(Source source) throws IOException
{
return null;
}
return Double.parseDouble(string);
+ case DECIMAL:
+ if (string.length() == 0) {
+ return null;
+ }
+ BigDecimal result = new BigDecimal(string);
+ if (result.precision() > fieldType.getPrecision() || result.scale() !=
fieldType
+ .getScale()) {
+ throw new IllegalArgumentException(
Review comment:
Does throwing make sense here? Or should we coerce?
##########
File path:
file/src/test/java/org/apache/calcite/adapter/file/FileAdapterTest.java
##########
@@ -619,6 +621,72 @@ private String range(int first, int count) {
return sb.append(')').toString();
}
+ @Test void testDecimalType() {
+ sql("sales-csv", "select BUDGET from sales.\"DECIMAL\"")
+ .checking(resultSet -> {
+ try {
+ ResultSetMetaData metaData = resultSet.getMetaData();
+ assertEquals("DECIMAL", metaData.getColumnTypeName(1));
+ assertEquals(18, metaData.getPrecision(1));
+ assertEquals(2, metaData.getScale(1));
+ } catch (SQLException e) {
+ throw TestUtil.rethrow(e);
+ }
+ })
+ .ok();
+ }
+
+ @Test void testDecimalTypeArithmeticOperations() {
Review comment:
Can you also test cases for negative cases and out of space numbers? For
example a CSV has 0.0000000000001 or 10000 but the it is declared as
DECIMAL(4,1).
##########
File path: file/src/main/java/org/apache/calcite/adapter/file/CsvEnumerator.java
##########
@@ -130,30 +129,108 @@ public static RelDataType deduceRowType(JavaTypeFactory
typeFactory,
}
for (String string : strings) {
final String name;
- final CsvFieldType fieldType;
+ final RelDataType fieldType;
final int colon = string.indexOf(':');
if (colon >= 0) {
name = string.substring(0, colon);
String typeString = string.substring(colon + 1);
- fieldType = CsvFieldType.of(typeString);
- if (fieldType == null) {
- System.out.println("WARNING: Found unknown type: "
- + typeString + " in file: " + source.path()
- + " for column: " + name
- + ". Will assume the type of column is string");
+ Matcher decimalMatcher = DECIMAL_TYPE_PATTERN.matcher(typeString);
+ if (decimalMatcher.matches()) {
+ int precision = Integer.parseInt(decimalMatcher.group(1));
+ int scale = Integer.parseInt(decimalMatcher.group(2));
+ fieldType = typeFactory.createTypeWithNullability(typeFactory
+ .createSqlType(SqlTypeName.DECIMAL,
+ precision, scale), true);
+ } else {
+ switch (typeString) {
+ case "string":
+ fieldType = typeFactory.createTypeWithNullability(typeFactory
+
.createSqlType(typeFactory.createJavaType(String.class).getSqlTypeName()),
Review comment:
Please just use the SqlTypeName enum directly as opposed to the
intermediate step of createJavaType()
##########
File path: file/src/test/resources/sales-csv/DECIMAL.csv
##########
@@ -0,0 +1,4 @@
+DEPTNO:int,BUDGET:decimal(18/2)
Review comment:
The standard pattern in most sql systems is a comma between scale and
precision. It's really weird that you're using a slash. Can you make this a
comma?
##########
File path:
core/src/main/java/org/apache/calcite/sql/type/SqlTypeFactoryImpl.java
##########
@@ -84,6 +84,10 @@ public SqlTypeFactoryImpl(RelDataTypeSystem typeSystem) {
|| (precision == RelDataType.PRECISION_NOT_SPECIFIED);
final int maxPrecision = typeSystem.getMaxPrecision(typeName);
if (maxPrecision >= 0 && precision > maxPrecision) {
+ System.out.println("WARNING: specified precision (" + precision
Review comment:
You shouldn't add this here. There are several reasons:
- This path is used by many pieces of code.
- We should never println in core paths to warn someone of something. It
could make sense to add a logging invocation here. However, I'm not sure it
does. If we thought that made sense, I think you should do it in a separate PR
since it is pretty unrelated to this change. If you want to capture and warn
that a field in the csv is out of line with what is allowed, you should do that
in the csv connector itself, not somewhere in the core module.
##########
File path: file/src/main/java/org/apache/calcite/adapter/file/CsvEnumerator.java
##########
@@ -130,30 +129,108 @@ public static RelDataType deduceRowType(JavaTypeFactory
typeFactory,
}
for (String string : strings) {
final String name;
- final CsvFieldType fieldType;
+ final RelDataType fieldType;
final int colon = string.indexOf(':');
if (colon >= 0) {
name = string.substring(0, colon);
String typeString = string.substring(colon + 1);
- fieldType = CsvFieldType.of(typeString);
- if (fieldType == null) {
- System.out.println("WARNING: Found unknown type: "
- + typeString + " in file: " + source.path()
- + " for column: " + name
- + ". Will assume the type of column is string");
+ Matcher decimalMatcher = DECIMAL_TYPE_PATTERN.matcher(typeString);
+ if (decimalMatcher.matches()) {
+ int precision = Integer.parseInt(decimalMatcher.group(1));
+ int scale = Integer.parseInt(decimalMatcher.group(2));
+ fieldType = typeFactory.createTypeWithNullability(typeFactory
+ .createSqlType(SqlTypeName.DECIMAL,
+ precision, scale), true);
+ } else {
+ switch (typeString) {
+ case "string":
+ fieldType = typeFactory.createTypeWithNullability(typeFactory
+
.createSqlType(typeFactory.createJavaType(String.class).getSqlTypeName()),
+ true);
+ break;
+ case "boolean":
+ fieldType = typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(
+
typeFactory.createJavaType(Primitive.BOOLEAN.getBoxClass()).getSqlTypeName()),
+ true);
+ break;
+ case "byte":
+ fieldType = typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(
+
typeFactory.createJavaType(Primitive.BYTE.getBoxClass()).getSqlTypeName()),
+ true);
+ break;
+ case "char":
+ fieldType = typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(
+
typeFactory.createJavaType(Primitive.CHAR.getBoxClass()).getSqlTypeName()),
+ true);
+ break;
+ case "short":
+ fieldType = typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(
+
typeFactory.createJavaType(Primitive.SHORT.getBoxClass()).getSqlTypeName()),
+ true);
+ break;
+ case "int":
+ fieldType = typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(
+
typeFactory.createJavaType(Primitive.INT.getBoxClass()).getSqlTypeName()),
+ true);
+ break;
+ case "long":
+ fieldType = typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(
+
typeFactory.createJavaType(Primitive.LONG.getBoxClass()).getSqlTypeName()),
+ true);
+ break;
+ case "float":
+ fieldType = typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(
+
typeFactory.createJavaType(Primitive.FLOAT.getBoxClass()).getSqlTypeName()),
+ true);
+ break;
+ case "double":
+ fieldType = typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(
+
typeFactory.createJavaType(Primitive.DOUBLE.getBoxClass()).getSqlTypeName()),
+ true);
+ break;
+ case "date":
+ fieldType = typeFactory.createTypeWithNullability(typeFactory
+
.createSqlType(typeFactory.createJavaType(java.sql.Date.class)
+ .getSqlTypeName()),
+ true);
+ break;
+ case "time":
+ fieldType = typeFactory.createTypeWithNullability(typeFactory
+
.createSqlType(typeFactory.createJavaType(java.sql.Time.class)
+ .getSqlTypeName()),
+ true);
+ break;
+ case "timestamp":
+ fieldType = typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(
+
typeFactory.createJavaType(java.sql.Timestamp.class).getSqlTypeName()),
+ true);
+ break;
+ default:
+ System.out.println("WARNING: Found unknown type: "
Review comment:
I think this should be converted into a logging statement as opposed to
a system.out (I know it was here before this change but we should clean this
kind of thing up when working on the code.)
##########
File path: file/src/main/java/org/apache/calcite/adapter/file/CsvEnumerator.java
##########
@@ -284,6 +361,17 @@ static CSVReader openCsv(Source source) throws IOException
{
return null;
}
return Double.parseDouble(string);
+ case DECIMAL:
+ if (string.length() == 0) {
+ return null;
+ }
+ BigDecimal result = new BigDecimal(string);
+ if (result.precision() > fieldType.getPrecision() || result.scale() !=
fieldType
+ .getScale()) {
+ throw new IllegalArgumentException(
Review comment:
Also, best to use string format for this kind of thing as opposed to
string concatenation.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
