FANNG1 commented on code in PR #4925:
URL: https://github.com/apache/gravitino/pull/4925#discussion_r1772898568
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/iceberg/IcebergMetadataAdapter.java:
##########
@@ -126,20 +120,12 @@ public GravitinoTable createTable(ConnectorTableMetadata
tableMetadata) {
new GravitinoTable(schemaName, tableName, columns, comment,
properties);
if (!partitionColumns.isEmpty()) {
- Transform[] partitioning =
-
partitionColumns.stream().map(Transforms::identity).toArray(Transform[]::new);
+ Transform[] partitioning =
ExpressionUtil.partitionFiledToExpression(partitionColumns);
Review Comment:
suggest to use a more meaning ful name, like `toGravitinoPartition`
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/iceberg/IcebergMetadataAdapter.java:
##########
@@ -126,20 +120,12 @@ public GravitinoTable createTable(ConnectorTableMetadata
tableMetadata) {
new GravitinoTable(schemaName, tableName, columns, comment,
properties);
if (!partitionColumns.isEmpty()) {
- Transform[] partitioning =
-
partitionColumns.stream().map(Transforms::identity).toArray(Transform[]::new);
+ Transform[] partitioning =
ExpressionUtil.partitionFiledToExpression(partitionColumns);
gravitinoTable.setPartitioning(partitioning);
}
if (!sortColumns.isEmpty()) {
- SortOrder[] sorting =
- sortColumns.stream()
- .map(
- sortingColumn -> {
- Expression expression =
NamedReference.field(sortingColumn);
- return SortOrders.ascending(expression);
- })
- .toArray(SortOrder[]::new);
+ SortOrder[] sorting =
ExpressionUtil.sortOrderFiledToExpression(sortColumns);
Review Comment:
suggest to use a more meaning ful name, like `toGravitinoSortOrder`
##########
trino-connector/integration-test/src/test/resources/trino-ci-testset/testsets/lakehouse-iceberg/00005_partition_sort_order.sql:
##########
@@ -0,0 +1,63 @@
+CREATE SCHEMA gt_db2;
+
+USE gt_db2;
+
+CREATE TABLE lineitem(
+ orderkey bigint,
+ partkey bigint,
+ suppkey bigint,
+ linenumber integer,
+ quantity decimal(12, 2),
+ extendedprice decimal(12, 2),
+ discount decimal(12, 2),
+ tax decimal(12, 2),
+ returnflag varchar,
+ linestatus varchar,
+ shipdate date,
+ commitdate date,
+ receiptdate date,
+ shipinstruct varchar,
+ shipmode varchar,
+ comment varchar
+)
+WITH (
+ partitioning = ARRAY['year(commitdate)'],
+ sorted_by = ARRAY['partkey', 'extendedprice desc']
+);
+
+show create table lineitem;
+
+insert into lineitem select * from tpch.tiny.lineitem;
+
+select * from lineitem order by orderkey, partkey limit 5;
+
+CREATE TABLE tb01(
+ orderkey bigint,
+ partkey bigint,
+ suppkey bigint,
+ linenumber integer,
+ quantity decimal(12, 2),
+ extendedprice decimal(12, 2),
+ discount decimal(12, 2),
+ tax decimal(12, 2),
+ returnflag varchar,
+ linestatus varchar,
+ shipdate date,
+ commitdate date,
+ receiptdate date,
+ shipinstruct varchar,
+ shipmode varchar,
+ comment varchar
+)
+WITH (
Review Comment:
how to check the partition or sort order takes effect?
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/iceberg/ExpressionUtil.java:
##########
@@ -0,0 +1,308 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.trino.connector.catalog.iceberg;
+
+import static
org.apache.gravitino.trino.connector.GravitinoErrorCode.GRAVITINO_EXPRESSION_ERROR;
+
+import io.trino.spi.TrinoException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.regex.MatchResult;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import org.apache.gravitino.rel.expressions.Expression;
+import org.apache.gravitino.rel.expressions.NamedReference;
+import org.apache.gravitino.rel.expressions.literals.Literal;
+import org.apache.gravitino.rel.expressions.sorts.NullOrdering;
+import org.apache.gravitino.rel.expressions.sorts.SortDirection;
+import org.apache.gravitino.rel.expressions.sorts.SortOrder;
+import org.apache.gravitino.rel.expressions.sorts.SortOrders;
+import org.apache.gravitino.rel.expressions.transforms.Transform;
+import org.apache.gravitino.rel.expressions.transforms.Transforms;
+
+/** This class is used to convert expression of bucket, sort_by, partition
object to string */
+public class ExpressionUtil {
+ private static final String IDENTIFIER = "[a-zA-Z_][a-zA-Z0-9_]*";
+ private static final String FUNCTION_ARG_INT = "(\\d+)";
+ private static final String FUNCTION_ARG_IDENTIFIER = "(" + IDENTIFIER + ")";
+ private static final Pattern YEAR_FUNCTION_PATTERN =
+ Pattern.compile("year\\(" + FUNCTION_ARG_IDENTIFIER + "\\)",
Pattern.CASE_INSENSITIVE);
+ private static final Pattern MONTH_FUNCTION_PATTERN =
+ Pattern.compile("month\\(" + FUNCTION_ARG_IDENTIFIER + "\\)",
Pattern.CASE_INSENSITIVE);
+ private static final Pattern DAY_FUNCTION_PATTERN =
+ Pattern.compile("day\\(" + FUNCTION_ARG_IDENTIFIER + "\\)",
Pattern.CASE_INSENSITIVE);
+ private static final Pattern HOUR_FUNCTION_PATTERN =
+ Pattern.compile("hour\\(" + FUNCTION_ARG_IDENTIFIER + "\\)",
Pattern.CASE_INSENSITIVE);
+ private static final Pattern BUCKET_FUNCTION_PATTERN =
+ Pattern.compile(
+ "bucket\\(" + FUNCTION_ARG_IDENTIFIER + ",\\s*" + FUNCTION_ARG_INT +
"\\)",
+ Pattern.CASE_INSENSITIVE);
+ private static final Pattern TRUNCATE_FUNCTION_PATTERN =
+ Pattern.compile(
+ "truncate\\(" + FUNCTION_ARG_IDENTIFIER + ",\\s*" + FUNCTION_ARG_INT
+ "\\)",
+ Pattern.CASE_INSENSITIVE);
+ private static final Pattern IDENTIFILER_PATTERN =
+ Pattern.compile(IDENTIFIER, Pattern.CASE_INSENSITIVE);
+
+ private static final String SORT_DIRECTION_ASC = "ASC";
+ private static final String SORT_DIRECTION_DESC = "DESC";
+ private static final String NULL_ORDERING_FIRST = "NULLS FIRST";
+ private static final String NULL_ORDERING_LAST = "NULLS LAST";
+ private static final String SORT_DIRECTION =
+ "(" + SORT_DIRECTION_ASC + "|" + SORT_DIRECTION_DESC + ")";
+ private static final String NULL_ORDERING =
+ "(" + NULL_ORDERING_FIRST + "|" + NULL_ORDERING_LAST + ")";
+ private static final Pattern SROT_ORDER_PATTERN =
+ Pattern.compile(IDENTIFIER, Pattern.CASE_INSENSITIVE);
+ private static final Pattern SORT_ORDER_WITH_SORT_DIRECTION_PATTERN =
+ Pattern.compile("(" + IDENTIFIER + ")\\s+" + SORT_DIRECTION,
Pattern.CASE_INSENSITIVE);
+ private static final Pattern
SORT_ORDER_WITH_SORT_DIRECTION_AND_NULL_ORDERING_PATTERN =
+ Pattern.compile(
+ "(" + IDENTIFIER + ")\\s+" + SORT_DIRECTION + "\\s+" + NULL_ORDERING,
+ Pattern.CASE_INSENSITIVE);
+
+ public static List<String> expressionToPartitionFiled(Transform[]
transforms) {
+ try {
+ List<String> partitionFields = new ArrayList<>();
+ for (Transform transform : transforms) {
+ partitionFields.add(transFormToString(transform));
+ }
+ return partitionFields;
+ } catch (IllegalArgumentException e) {
+ throw new TrinoException(
+ GRAVITINO_EXPRESSION_ERROR,
+ "Error to handle transform Expressions :" + e.getMessage(),
+ e);
+ }
+ }
+
+ public static Transform[] partitionFiledToExpression(List<String>
partitions) {
+ try {
+ List<Transform> partitionTransforms = new ArrayList<>();
+ for (String partition : partitions) {
+ parseTransform(partitionTransforms, partition);
+ }
+ return partitionTransforms.toArray(new Transform[0]);
+ } catch (IllegalArgumentException e) {
+ throw new TrinoException(
+ GRAVITINO_EXPRESSION_ERROR, "Error parsing the partition field: " +
e.getMessage(), e);
+ }
+ }
+
+ public static List<String> expressionToSortOrderFiled(SortOrder[] orders) {
+ try {
+ List<String> orderFields = new ArrayList<>();
+ for (SortOrder order : orders) {
+ orderFields.add(sortOrderToString(order));
+ }
+ return orderFields;
+ } catch (IllegalArgumentException e) {
+ throw new TrinoException(
+ GRAVITINO_EXPRESSION_ERROR,
+ "Error to handle the sort order expressions : " + e.getMessage(),
+ e);
+ }
+ }
+
+ public static SortOrder[] sortOrderFiledToExpression(List<String>
orderFields) {
+ try {
+ List<SortOrder> sortOrders = new ArrayList<>();
+ for (String orderField : orderFields) {
+ parseSortOrder(sortOrders, orderField);
+ }
+ return sortOrders.toArray(new SortOrder[0]);
+ } catch (IllegalArgumentException e) {
+ throw new TrinoException(
+ GRAVITINO_EXPRESSION_ERROR, "Error parsing the sort order field: " +
e.getMessage(), e);
+ }
+ }
+
+ private static void parseTransform(List<Transform> transforms, String value)
{
Review Comment:
it's hard to maintan, is there any any better way to implement this? how
origin Iceberg connector handle this logic?
##########
trino-connector/integration-test/build.gradle.kts:
##########
@@ -70,17 +70,24 @@ dependencies {
testRuntimeOnly(libs.junit.jupiter.engine)
}
+tasks.register("setupDependencies") {
+ dependsOn(":trino-connector:trino-connector:jar")
+ dependsOn(":catalogs:catalog-lakehouse-iceberg:jar",
":catalogs:catalog-lakehouse-iceberg:runtimeJars")
+ dependsOn(":catalogs:catalog-jdbc-mysql:jar",
":catalogs:catalog-jdbc-mysql:runtimeJars")
+ dependsOn(":catalogs:catalog-jdbc-postgresql:jar",
":catalogs:catalog-jdbc-postgresql:runtimeJars")
+ dependsOn(":catalogs:catalog-hive:jar", ":catalogs:catalog-hive:runtimeJars")
+}
+
+tasks.build {
+ dependsOn("setupDependencies")
Review Comment:
why build dependens on `setupDependencies `?
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/iceberg/IcebergMetadataAdapter.java:
##########
@@ -158,28 +144,15 @@ public ConnectorTableMetadata
getTableMetadata(GravitinoTable gravitinoTable) {
Map<String, Object> properties =
toTrinoTableProperties(gravitinoTable.getProperties());
if (ArrayUtils.isNotEmpty(gravitinoTable.getPartitioning())) {
- // Only support simple partition now like partition by a, b, c.
- // Format like partition like partition by year(a), b, c is NOT
supported now.
properties.put(
IcebergPropertyMeta.ICEBERG_PARTITIONING_PROPERTY,
- gravitinoTable.getPartitioning().length > 0
- ? Arrays.stream(gravitinoTable.getPartitioning())
- .map(ts -> ((Transform.SingleFieldTransform)
ts).fieldName()[0])
- .collect(Collectors.toList())
- : Collections.emptyList());
+
ExpressionUtil.expressionToPartitionFiled(gravitinoTable.getPartitioning()));
Review Comment:
use a more meaningful name like `toTrinoPartition`?
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/iceberg/IcebergMetadataAdapter.java:
##########
@@ -158,28 +144,15 @@ public ConnectorTableMetadata
getTableMetadata(GravitinoTable gravitinoTable) {
Map<String, Object> properties =
toTrinoTableProperties(gravitinoTable.getProperties());
if (ArrayUtils.isNotEmpty(gravitinoTable.getPartitioning())) {
- // Only support simple partition now like partition by a, b, c.
- // Format like partition like partition by year(a), b, c is NOT
supported now.
properties.put(
IcebergPropertyMeta.ICEBERG_PARTITIONING_PROPERTY,
- gravitinoTable.getPartitioning().length > 0
- ? Arrays.stream(gravitinoTable.getPartitioning())
- .map(ts -> ((Transform.SingleFieldTransform)
ts).fieldName()[0])
- .collect(Collectors.toList())
- : Collections.emptyList());
+
ExpressionUtil.expressionToPartitionFiled(gravitinoTable.getPartitioning()));
}
if (ArrayUtils.isNotEmpty(gravitinoTable.getSortOrders())) {
- // Only support the simple format
properties.put(
IcebergPropertyMeta.ICEBERG_SORTED_BY_PROPERTY,
- Arrays.stream(gravitinoTable.getSortOrders())
- .map(
- sortOrder -> {
- Expression expression = sortOrder.expression();
- return ((NamedReference) expression).fieldName()[0];
- })
- .collect(Collectors.toList()));
+
ExpressionUtil.expressionToSortOrderFiled(gravitinoTable.getSortOrders()));
Review Comment:
use a more meaningful name like `toTrinoSortOrder`?
--
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]
