Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support DECIMAL and remove DOUBLE in TPCH Connector #11166

Closed
wants to merge 3 commits into from
Closed

Support DECIMAL and remove DOUBLE in TPCH Connector #11166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2cd15c3
Fix TPCH connector to use DECIMAL type instead of DOUBLE to conform w…
havi-odin Jul 31, 2018
1235622
UNDO: Temp changes to pom
havi-odin Jul 31, 2018
0045297
Get existing tests working
havi-odin Aug 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,17 @@
<parent>
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes made to this file will be reverted prior to merging. Please ignore these changes

<groupId>io.airlift</groupId>
<artifactId>airbase</artifactId>
<version>80</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like you depends on other PR which is in progress, it should be a separate commit (cherry-picked)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see:
#11166 (comment)

It is hard for me to locally separate out this commit, and my local testing needs these changes. Hence I raised this as part of the PR, will revert before merging. Please ignore any changes to this file

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see:
#11166 (comment)

It is hard for me to locally separate out this commit, and my local testing needs these changes. Hence I raised this as part of the PR, will revert before merging. Please ignore any changes to this file

<version>82</version>
</parent>

<repositories>
<repository>
<id>atri-local</id>
<name>Local AirLift Repo</name>
<url>/Users/atrisharma/tpch/target</url>
</repository>
</repositories>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#11166 (comment)


<groupId>com.facebook.presto</groupId>
<artifactId>presto-root</artifactId>
<version>0.204-SNAPSHOT</version>
Expand Down Expand Up @@ -122,6 +130,11 @@

<dependencyManagement>
<dependencies>
<dependency>
<groupId>io.airlift.tpch</groupId>
<artifactId>tpch</artifactId>
<version>0.10-SNAPSHOT</version>
</dependency>
<dependency>
<groupId>com.facebook.presto</groupId>
<artifactId>presto-spi</artifactId>
Expand Down Expand Up @@ -598,12 +611,6 @@
<version>${dep.drift.version}</version>
</dependency>

<dependency>
<groupId>io.airlift.tpch</groupId>
<artifactId>tpch</artifactId>
<version>0.9</version>
</dependency>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not move

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#11166 (comment)

<dependency>
<groupId>com.teradata.tpcds</groupId>
<artifactId>tpcds</artifactId>
Expand Down
9 changes: 5 additions & 4 deletions presto-hive/src/test/java/com/facebook/presto/hive/TestHivePageSink.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
import static com.facebook.presto.hive.metastore.file.FileHiveMetastore.createTestingFileHiveMetastore;
import static com.facebook.presto.spi.type.BigintType.BIGINT;
import static com.facebook.presto.spi.type.DateType.DATE;
import static com.facebook.presto.spi.type.DecimalType.DECIMAL;
import static com.facebook.presto.spi.type.DoubleType.DOUBLE;
import static com.facebook.presto.spi.type.IntegerType.INTEGER;
import static com.facebook.presto.spi.type.VarcharType.createUnboundedVarcharType;
Expand Down Expand Up @@ -159,8 +160,8 @@ private static long writeTestFile(HiveClientConfig config, ExtendedHiveMetastore
case DATE:
DATE.writeLong(blockBuilder, column.getDate(lineItem));
break;
case DOUBLE:
DOUBLE.writeDouble(blockBuilder, column.getDouble(lineItem));
case DECIMAL:
DECIMAL.writeLong(blockBuilder, column.getIdentifier(lineItem));
break;
case VARCHAR:
createUnboundedVarcharType().writeSlice(blockBuilder, Slices.utf8Slice(column.getString(lineItem)));
Expand Down Expand Up @@ -302,8 +303,8 @@ private static HiveType getHiveType(TpchColumnType type)
return HIVE_INT;
case DATE:
return HIVE_DATE;
case DOUBLE:
return HIVE_DOUBLE;
case DECIMAL:
return HIVE_LONG;
case VARCHAR:
return HIVE_STRING;
default:
Expand Down
9 changes: 5 additions & 4 deletions presto-hive/src/test/java/com/facebook/presto/hive/benchmark/HiveFileFormatBenchmark.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
import static com.facebook.presto.hive.HiveTestUtils.mapType;
import static com.facebook.presto.spi.type.BigintType.BIGINT;
import static com.facebook.presto.spi.type.DateType.DATE;
import static com.facebook.presto.spi.type.DecimalType.DECIMAL;
import static com.facebook.presto.spi.type.DoubleType.DOUBLE;
import static com.facebook.presto.spi.type.IntegerType.INTEGER;
import static com.facebook.presto.spi.type.VarcharType.createUnboundedVarcharType;
Expand Down Expand Up @@ -497,8 +498,8 @@ private static <E extends TpchEntity> TestData createTpchDataSet(FileFormat form
createUnboundedVarcharType().writeString(blockBuilder, column.getString(row));
}
break;
case DOUBLE:
DOUBLE.writeDouble(blockBuilder, column.getDouble(row));
case DECIMAL:
DECIMAL.writeLong(blockBuilder, column.getIdentifier(row));
break;
case VARCHAR:
createUnboundedVarcharType().writeSlice(blockBuilder, Slices.utf8Slice(column.getString(row)));
Expand Down Expand Up @@ -568,8 +569,8 @@ private static Type getColumnType(TpchColumn<?> input)
return INTEGER;
case DATE:
return DATE;
case DOUBLE:
return DOUBLE;
case DECIMAL:
return DECIMAL;
case VARCHAR:
return createUnboundedVarcharType();
}
Expand Down
12 changes: 9 additions & 3 deletions ...to-main/src/test/java/com/facebook/presto/sql/planner/optimizations/TestMergeWindows.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,10 @@ public void testIdenticalWindowSpecificationsABcpA()
window(windowMatcherBuilder -> windowMatcherBuilder
.specification(specificationB)
.addFunction(functionCall("lag", COMMON_FRAME, ImmutableList.of(QUANTITY_ALIAS, "ONE", "ZERO"))),
project(ImmutableMap.of("ONE", expression("CAST(1 AS bigint)"), "ZERO", expression("0.0E0")),
project(ImmutableMap.of(
"quantity_3", expression("CAST(\"quantity\" AS double)"),
"ONE", expression("CAST(1 AS bigint)"),
"ZERO", expression("0.0E0")),
LINEITEM_TABLESCAN_DOQSS)))));
}

Expand Down Expand Up @@ -254,7 +257,10 @@ public void testIdenticalWindowSpecificationsAAcpA()
.addFunction(functionCall("sum", COMMON_FRAME, ImmutableList.of(DISCOUNT_ALIAS)))
.addFunction(functionCall("lag", COMMON_FRAME, ImmutableList.of(QUANTITY_ALIAS, "ONE", "ZERO")))
.addFunction(functionCall("sum", COMMON_FRAME, ImmutableList.of(QUANTITY_ALIAS))),
project(ImmutableMap.of("ONE", expression("CAST(1 AS bigint)"), "ZERO", expression("0.0E0")),
project(ImmutableMap.of(
"quantity_3", expression("CAST(\"quantity\" AS double)"),
"ONE", expression("CAST(1 AS bigint)"),
"ZERO", expression("0.0E0")),
LINEITEM_TABLESCAN_DOQS))));
}

Expand Down Expand Up @@ -434,7 +440,7 @@ public void testNotMergeAcrossJoinBranches()

assertUnitPlan(sql,
anyTree(
filter("SUM = AVG",
filter("SUM = CAST(\"avg\" AS decimal(38,2))",
join(JoinNode.Type.INNER, ImmutableList.of(),
any(
window(windowMatcherBuilder -> windowMatcherBuilder
Expand Down
1 change: 1 addition & 0 deletions presto-spi/src/main/java/com/facebook/presto/spi/type/DecimalType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ public abstract class DecimalType
{
public static final int DEFAULT_SCALE = 0;
public static final int DEFAULT_PRECISION = MAX_PRECISION;
public static final DecimalType DECIMAL = createDecimalType(12, 2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please undo

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Types usually have a static created type (e.g. DOUBLE etc), hence I created one for DECIMAL. I will use createDecimal everywhere instead

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was following the usual pattern of types having a static instance which can be returned. I will use createDecimal


public static DecimalType createDecimalType(int precision, int scale)
{
Expand Down
10 changes: 7 additions & 3 deletions presto-tpch/src/main/java/com/facebook/presto/tpch/TpchMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@

import static com.facebook.presto.spi.type.BigintType.BIGINT;
import static com.facebook.presto.spi.type.DateType.DATE;
import static com.facebook.presto.spi.type.DecimalType.DECIMAL;
import static com.facebook.presto.spi.type.DoubleType.DOUBLE;
import static com.facebook.presto.spi.type.IntegerType.INTEGER;
import static com.facebook.presto.spi.type.VarcharType.createVarcharType;
Expand Down Expand Up @@ -386,7 +387,10 @@ private Object toPrestoValue(Object tpchValue, Type columnType)
if (columnType instanceof VarcharType) {
return Slices.utf8Slice((String) tpchValue);
}
if (columnType.equals(BIGINT) || columnType.equals(INTEGER) || columnType.equals(DATE)) {
if (columnType.equals(BIGINT)
|| columnType.equals(INTEGER)
|| columnType.equals(DATE)
|| columnType.equals(DECIMAL)) {
return ((Number) tpchValue).longValue();
}
if (columnType.equals(DOUBLE)) {
Expand Down Expand Up @@ -487,8 +491,8 @@ public static Type getPrestoType(TpchColumn<?> column)
return INTEGER;
case DATE:
return DATE;
case DOUBLE:
return DOUBLE;
case DECIMAL:
return DECIMAL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should return DECIMAL(12,2), use com.facebook.presto.spi.type.DecimalType#createDecimalType(int, int)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

case VARCHAR:
return createVarcharType((int) (long) tpchType.getPrecision().get());
}
Expand Down
4 changes: 2 additions & 2 deletions presto-tpch/src/test/java/com/facebook/presto/tpch/statistics/TableStatisticsRecorder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ private <E extends TpchEntity> Comparable<?> getTpchValue(E row, TpchColumn<E> c
return column.getInteger(row);
case DATE:
return column.getDate(row);
case DOUBLE:
return column.getDouble(row);
case DECIMAL:
return column.getIdentifier(row);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you mean long?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getIdentifier returns the long value of the column, should it not work for this case?

case VARCHAR:
return column.getString(row);
}
Expand Down