You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/06/29 14:08:26 UTC
[GitHub] [iceberg] sshkvar opened a new pull request #2757: Add support for reading/writing timestamps without timezone.
sshkvar opened a new pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757
Creating PR from my forked repo as discussed https://github.com/apache/iceberg/pull/2282
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r661534486
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkUtil.java
##########
@@ -20,21 +20,38 @@
package org.apache.iceberg.spark;
import java.util.List;
+import java.util.Map;
import java.util.function.BiFunction;
import java.util.function.Function;
import java.util.stream.Collectors;
import org.apache.iceberg.PartitionField;
import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
import org.apache.iceberg.Table;
import org.apache.iceberg.hadoop.HadoopFileIO;
import org.apache.iceberg.io.FileIO;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.transforms.Transform;
import org.apache.iceberg.transforms.UnknownTransform;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.Pair;
+import org.apache.spark.sql.RuntimeConfig;
import org.apache.spark.util.SerializableConfiguration;
public class SparkUtil {
+
+ public static final String HANDLE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY =
+ "spark.sql.iceberg.convert-timestamp-without-timezone";
+ public static final String TIMESTAMP_WITHOUT_TIMEZONE_ERROR = String.format("Cannot handle timestamp without" +
+ " timezone fields in Spark. Spark does not natively support this type but if you would like to handle all" +
+ " timestamps as timestamp with timezone set '%s' to true. This will not change the underlying values stored" +
+ " but will change their displayed values in Spark. For more information please see" +
+ " https://docs.databricks.com/spark/latest/dataframes-datasets/dates-timestamps.html#ansi-sql-and" +
+ "-spark-sql-timestamps", HANDLE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY);
+ public static final String READ_TIMESTAMP_AS_TIMESTAMP_WITHOUT_TIMEZONE =
Review comment:
I'm not sure I follow based on the names, we have one property for reading and writing to existing tables, and you would like there to be an additional property for creating new tables which defaults the iceberg table to TS without Timezone?
If so can we just make those names a little clearer?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r661580931
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkFixupTimestampType.java
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.iceberg.spark;
+
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.types.FixupTypes;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
+
+/**
+ * By default Spark TimestampType should be converted to Types.TimestampType.withZone() iceberg type.
+ * But we also can convert TimestampType to Types.TimestampType.withoutZone() iceberg type
+ * by setting SparkUtil.STORE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY to 'true'
+ */
+class SparkFixupTimestampType extends FixupTypes {
+
+ private final boolean useTimestampWithoutZone;
+
+ private SparkFixupTimestampType(Schema referenceSchema, boolean useTimestampWithoutZone) {
Review comment:
Ok so basically there is "Read and write to existing tables regardless of timezone type" vs "Create new tables using non-spark timezone type". Let's make sure we have tests for both parameters
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#issuecomment-878247091
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r664660277
##########
File path: spark/src/test/java/org/apache/iceberg/spark/source/TestTimestampWithoutZone.java
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.iceberg.spark.source;
+
+import java.io.File;
+import java.io.IOException;
+import java.time.LocalDateTime;
+import java.util.List;
+import java.util.Locale;
+import java.util.UUID;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.AssertHelpers;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DataFiles;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.data.GenericAppenderFactory;
+import org.apache.iceberg.data.GenericRecord;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.hadoop.HadoopTables;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.spark.SparkUtil;
+import org.apache.iceberg.spark.data.GenericsHelpers;
+import org.apache.iceberg.types.Types;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.SaveMode;
+import org.apache.spark.sql.SparkSession;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import static org.apache.iceberg.Files.localOutput;
+
+@RunWith(Parameterized.class)
+public abstract class TestTimestampWithoutZone {
+ private static final Configuration CONF = new Configuration();
+ private static final HadoopTables TABLES = new HadoopTables(CONF);
+
+ private static final Schema SCHEMA = new Schema(
+ Types.NestedField.required(1, "id", Types.LongType.get()),
+ Types.NestedField.optional(2, "ts", Types.TimestampType.withoutZone()),
+ Types.NestedField.optional(3, "data", Types.StringType.get())
+ );
+
+ private static SparkSession spark = null;
+
+ @BeforeClass
+ public static void startSpark() {
+ TestTimestampWithoutZone.spark = SparkSession.builder().master("local[2]").getOrCreate();
+ }
+
+ @AfterClass
+ public static void stopSpark() {
+ SparkSession currentSpark = TestTimestampWithoutZone.spark;
+ TestTimestampWithoutZone.spark = null;
+ currentSpark.stop();
+ }
+
+ @Rule
+ public TemporaryFolder temp = new TemporaryFolder();
+
+ private final String format;
+ private final boolean vectorized;
+
+ @Parameterized.Parameters(name = "format = {0}, vectorized = {1}")
+ public static Object[][] parameters() {
+ return new Object[][] {
+ { "parquet", false },
+ { "parquet", true },
+ { "avro", false }
+ };
+ }
+
+ public TestTimestampWithoutZone(String format, boolean vectorized) {
+ this.format = format;
+ this.vectorized = vectorized;
+ }
+
+ private File parent = null;
+ private File unpartitioned = null;
+ private List<Record> records = null;
+
+ @Before
+ public void writeUnpartitionedTable() throws IOException {
+ this.parent = temp.newFolder("TestTimestampWithoutZone");
+ this.unpartitioned = new File(parent, "unpartitioned");
+ File dataFolder = new File(unpartitioned, "data");
+ Assert.assertTrue("Mkdir should succeed", dataFolder.mkdirs());
+
+ Table table = TABLES.create(SCHEMA, PartitionSpec.unpartitioned(), unpartitioned.toString());
+ Schema tableSchema = table.schema(); // use the table schema because ids are reassigned
+
+ FileFormat fileFormat = FileFormat.valueOf(format.toUpperCase(Locale.ENGLISH));
+
+ File testFile = new File(dataFolder, fileFormat.addExtension(UUID.randomUUID().toString()));
+
+ // create records using the table's schema
+ this.records = testRecords(tableSchema);
+
+ try (FileAppender<Record> writer = new GenericAppenderFactory(tableSchema).newAppender(
+ localOutput(testFile), fileFormat)) {
+ writer.addAll(records);
+ }
+
+ DataFile file = DataFiles.builder(PartitionSpec.unpartitioned())
+ .withRecordCount(records.size())
+ .withFileSizeInBytes(testFile.length())
+ .withPath(testFile.toString())
+ .build();
+
+ table.newAppend().appendFile(file).commit();
+ }
+
+ @Test
+ public void testUnpartitionedTimestampWithoutZone() {
+ assertEqualsSafe(SCHEMA.asStruct(), records, read(unpartitioned.toString(), vectorized));
+ }
+
+ @Test
+ public void testUnpartitionedTimestampWithoutZoneProjection() {
+ Schema projection = SCHEMA.select("id", "ts");
+ assertEqualsSafe(projection.asStruct(),
+ records.stream().map(r -> projectFlat(projection, r)).collect(Collectors.toList()),
+ read(unpartitioned.toString(), vectorized, "id", "ts"));
+ }
+
+ @Rule
+ public ExpectedException exception = ExpectedException.none();
+
+ @Test
+ public void testUnpartitionedTimestampWithoutZoneError() {
+ AssertHelpers.assertThrows(String.format("Read operation performed on a timestamp without timezone field while " +
+ "'%s' set to false should throw exception",
+ SparkUtil.HANDLE_TIMESTAMP_WITHOUT_TIMEZONE),
+ IllegalArgumentException.class, SparkUtil.TIMESTAMP_WITHOUT_TIMEZONE_ERROR, () -> spark.read()
Review comment:
nit: I would just put the function parameter on the next line
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r670204421
##########
File path: spark2/src/main/java/org/apache/iceberg/spark/source/Reader.java
##########
@@ -183,6 +187,8 @@ private Expression filterExpression() {
private StructType lazyType() {
if (type == null) {
+ Preconditions.checkArgument(readTimestampWithoutZone || !SparkUtil.hasTimestampWithoutZone(lazySchema()),
+ SparkUtil.TIMESTAMP_WITHOUT_TIMEZONE_ERROR);
Review comment:
I think no, because we need this check only in few places, while SparkSchemaUtil.convert used in a lot of places
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r668510746
##########
File path: spark/src/test/java/org/apache/iceberg/spark/data/AvroDataTest.java
##########
@@ -185,4 +188,18 @@ public void testMixedTypes() throws IOException {
writeAndValidate(schema);
}
+
+ @Test
+ public void testTimestampWithoutZone() throws IOException {
+ Option<SparkSession> sparkSession = SparkSession.getActiveSession();
+ if (sparkSession.isDefined()) {
+ sparkSession.get().conf().set(SparkUtil.HANDLE_TIMESTAMP_WITHOUT_TIMEZONE, "true");
+ }
Review comment:
good point, I changed the code based on your recommendations
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r661504713
##########
File path: spark/src/main/java/org/apache/iceberg/spark/TypeToSparkType.java
##########
@@ -104,12 +104,14 @@ public DataType primitive(Type.PrimitiveType primitive) {
throw new UnsupportedOperationException(
"Spark does not support time fields");
case TIMESTAMP:
- Types.TimestampType timestamp = (Types.TimestampType) primitive;
- if (timestamp.shouldAdjustToUTC()) {
- return TimestampType$.MODULE$;
- }
- throw new UnsupportedOperationException(
- "Spark does not support timestamp without time zone fields");
+// Types.TimestampType timestamp = (Types.TimestampType) primitive;
Review comment:
done
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r661504527
##########
File path: spark/src/test/java/org/apache/iceberg/spark/data/AvroDataTest.java
##########
@@ -40,6 +42,8 @@
protected abstract void writeAndValidate(Schema schema) throws IOException;
+ protected abstract SparkSession getSparkSession();
Review comment:
Agree, I have removed `getSparkSession()` abstract method, and just used `SparkSession.getActive`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r662033631
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkUtil.java
##########
@@ -100,4 +117,55 @@ public static void validatePartitionTransforms(PartitionSpec spec) {
}
}
}
+
+ /**
+ * Responsible for checking if the table schema has a timestamp without timezone column
+ * @param schema table schema to check if it contains a timestamp without timezone column
+ * @return boolean indicating if the schema passed in has a timestamp field without a timezone
+ */
+ public static boolean hasTimestampWithoutZone(Schema schema) {
+ return TypeUtil.find(schema, t -> Types.TimestampType.withoutZone().equals(t)) != null;
+ }
+
+ /**
+ * Allow reading timestamp without time zone as timestamp with time zone. Generally, this is not safe as timestamp
+ * without time zone is supposed to represent wall clock time semantics, i.e. no matter the reader/writer timezone
+ * 3PM should always be read as 3PM, but timestamp with time zone represents instant semantics, i.e the timestamp
+ * is adjusted so that the corresponding time in the reader timezone is displayed.
+ * When set to false (default), we throw an exception at runtime
+ * "Spark does not support timestamp without time zone fields" if reading timestamp without time zone fields
+ *
+ * @param readerConfig table read options
+ * @param sessionConf spark session configurations
+ * @return boolean indicating if reading timestamps without timezone is allowed
+ */
+ public static boolean canHandleTimestampWithoutZone(Map<String, String> readerConfig, RuntimeConfig sessionConf) {
+ String readerOption = readerConfig.get(HANDLE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY);
+ if (readerOption != null) {
+ return Boolean.parseBoolean(readerOption);
+ }
+ String sessionConfValue = sessionConf.get(HANDLE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY, null);
+ if (sessionConfValue != null) {
+ return Boolean.parseBoolean(sessionConfValue);
+ }
+ return false;
+ }
+
+ /**
+ * Check whether the spark session config contains a 'spark.sql.iceberg.read-timestamp-as-timestamp-without-timezone'
+ * property.
+ * Default value - false
+ *
+ * @param sessionConf a spark runtime config
+ * @return true if the session config has spark.sql.iceberg.read-timestamp-as-timestamp-without-timezone property
Review comment:
done
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r670205582
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkFixupTimestampType.java
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.iceberg.spark;
+
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.types.FixupTypes;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
+
+/**
+ * By default Spark type {@link org.apache.iceberg.types.Types.TimestampType} should be converted to
+ * {@link Types.TimestampType#withZone()} iceberg type. But we also can convert
+ * {@link org.apache.iceberg.types.Types.TimestampType} to {@link Types.TimestampType#withoutZone()} iceberg type
+ * by setting {@link SparkUtil#USE_TIMESTAMP_WITHOUT_TIME_ZONE_IN_NEW_TABLES} to 'true'
+ */
+class SparkFixupTimestampType extends FixupTypes {
+
+ private SparkFixupTimestampType(Schema referenceSchema) {
+ super(referenceSchema);
+ }
+
+ static Schema fixup(Schema schema) {
+ return new Schema(TypeUtil.visit(schema,
+ new SparkFixupTimestampType(schema)).asStructType().fields());
Review comment:
This is the plug-in that is used in general for your info. Specifically, this one would be the checkstyle one, but we also use error prone as well: https://github.com/palantir/gradle-baseline
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r662035283
##########
File path: spark3/src/main/java/org/apache/iceberg/spark/source/SparkBatchScan.java
##########
@@ -158,8 +166,10 @@ public PartitionReaderFactory createReaderFactory() {
boolean batchReadsEnabled = batchReadsEnabled(allParquetFileScanTasks, allOrcFileScanTasks);
+ boolean hasTimestampWithoutZone = SparkUtil.hasTimestampWithoutZone(expectedSchema);
+
boolean readUsingBatch = batchReadsEnabled && hasNoDeleteFiles && (allOrcFileScanTasks ||
- (allParquetFileScanTasks && atLeastOneColumn && onlyPrimitives));
+ (allParquetFileScanTasks && atLeastOneColumn && onlyPrimitives && !hasTimestampWithoutZone));
Review comment:
I have removed this check
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#issuecomment-880106984
I can trigger a re-run, just pinged @karuppayya to take a look
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r667812697
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkUtil.java
##########
@@ -20,21 +20,38 @@
package org.apache.iceberg.spark;
import java.util.List;
+import java.util.Map;
import java.util.function.BiFunction;
import java.util.function.Function;
import java.util.stream.Collectors;
import org.apache.iceberg.PartitionField;
import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
import org.apache.iceberg.Table;
import org.apache.iceberg.hadoop.HadoopFileIO;
import org.apache.iceberg.io.FileIO;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.transforms.Transform;
import org.apache.iceberg.transforms.UnknownTransform;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.Pair;
+import org.apache.spark.sql.RuntimeConfig;
import org.apache.spark.util.SerializableConfiguration;
public class SparkUtil {
+
+ public static final String HANDLE_TIMESTAMP_WITHOUT_TIMEZONE =
+ "spark.sql.iceberg.convert-timestamp-without-timezone";
Review comment:
done
##########
File path: spark/src/test/java/org/apache/iceberg/spark/source/TestTimestampWithoutZone.java
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.iceberg.spark.source;
+
+import java.io.File;
+import java.io.IOException;
+import java.time.LocalDateTime;
+import java.util.List;
+import java.util.Locale;
+import java.util.UUID;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.AssertHelpers;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DataFiles;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.data.GenericAppenderFactory;
+import org.apache.iceberg.data.GenericRecord;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.hadoop.HadoopTables;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.spark.SparkUtil;
+import org.apache.iceberg.spark.data.GenericsHelpers;
+import org.apache.iceberg.types.Types;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.SaveMode;
+import org.apache.spark.sql.SparkSession;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import static org.apache.iceberg.Files.localOutput;
+
+@RunWith(Parameterized.class)
+public abstract class TestTimestampWithoutZone {
+ private static final Configuration CONF = new Configuration();
+ private static final HadoopTables TABLES = new HadoopTables(CONF);
+
+ private static final Schema SCHEMA = new Schema(
+ Types.NestedField.required(1, "id", Types.LongType.get()),
+ Types.NestedField.optional(2, "ts", Types.TimestampType.withoutZone()),
+ Types.NestedField.optional(3, "data", Types.StringType.get())
+ );
+
+ private static SparkSession spark = null;
+
+ @BeforeClass
+ public static void startSpark() {
+ TestTimestampWithoutZone.spark = SparkSession.builder().master("local[2]").getOrCreate();
+ }
+
+ @AfterClass
+ public static void stopSpark() {
+ SparkSession currentSpark = TestTimestampWithoutZone.spark;
+ TestTimestampWithoutZone.spark = null;
+ currentSpark.stop();
+ }
+
+ @Rule
+ public TemporaryFolder temp = new TemporaryFolder();
+
+ private final String format;
+ private final boolean vectorized;
+
+ @Parameterized.Parameters(name = "format = {0}, vectorized = {1}")
+ public static Object[][] parameters() {
+ return new Object[][] {
+ { "parquet", false },
+ { "parquet", true },
+ { "avro", false }
+ };
+ }
+
+ public TestTimestampWithoutZone(String format, boolean vectorized) {
+ this.format = format;
+ this.vectorized = vectorized;
+ }
+
+ private File parent = null;
+ private File unpartitioned = null;
+ private List<Record> records = null;
+
+ @Before
+ public void writeUnpartitionedTable() throws IOException {
+ this.parent = temp.newFolder("TestTimestampWithoutZone");
+ this.unpartitioned = new File(parent, "unpartitioned");
+ File dataFolder = new File(unpartitioned, "data");
+ Assert.assertTrue("Mkdir should succeed", dataFolder.mkdirs());
+
+ Table table = TABLES.create(SCHEMA, PartitionSpec.unpartitioned(), unpartitioned.toString());
+ Schema tableSchema = table.schema(); // use the table schema because ids are reassigned
+
+ FileFormat fileFormat = FileFormat.valueOf(format.toUpperCase(Locale.ENGLISH));
+
+ File testFile = new File(dataFolder, fileFormat.addExtension(UUID.randomUUID().toString()));
+
+ // create records using the table's schema
+ this.records = testRecords(tableSchema);
+
+ try (FileAppender<Record> writer = new GenericAppenderFactory(tableSchema).newAppender(
+ localOutput(testFile), fileFormat)) {
+ writer.addAll(records);
+ }
+
+ DataFile file = DataFiles.builder(PartitionSpec.unpartitioned())
+ .withRecordCount(records.size())
+ .withFileSizeInBytes(testFile.length())
+ .withPath(testFile.toString())
+ .build();
+
+ table.newAppend().appendFile(file).commit();
+ }
+
+ @Test
+ public void testUnpartitionedTimestampWithoutZone() {
+ assertEqualsSafe(SCHEMA.asStruct(), records, read(unpartitioned.toString(), vectorized));
+ }
+
+ @Test
+ public void testUnpartitionedTimestampWithoutZoneProjection() {
+ Schema projection = SCHEMA.select("id", "ts");
+ assertEqualsSafe(projection.asStruct(),
+ records.stream().map(r -> projectFlat(projection, r)).collect(Collectors.toList()),
+ read(unpartitioned.toString(), vectorized, "id", "ts"));
+ }
+
+ @Rule
+ public ExpectedException exception = ExpectedException.none();
+
+ @Test
+ public void testUnpartitionedTimestampWithoutZoneError() {
+ AssertHelpers.assertThrows(String.format("Read operation performed on a timestamp without timezone field while " +
+ "'%s' set to false should throw exception",
+ SparkUtil.HANDLE_TIMESTAMP_WITHOUT_TIMEZONE),
+ IllegalArgumentException.class, SparkUtil.TIMESTAMP_WITHOUT_TIMEZONE_ERROR, () -> spark.read()
Review comment:
resolved
##########
File path: spark3/src/main/java/org/apache/iceberg/spark/source/SparkBatchScan.java
##########
@@ -159,7 +167,7 @@ public PartitionReaderFactory createReaderFactory() {
boolean batchReadsEnabled = batchReadsEnabled(allParquetFileScanTasks, allOrcFileScanTasks);
boolean readUsingBatch = batchReadsEnabled && hasNoDeleteFiles && (allOrcFileScanTasks ||
- (allParquetFileScanTasks && atLeastOneColumn && onlyPrimitives));
+ (allParquetFileScanTasks && atLeastOneColumn && onlyPrimitives));
Review comment:
removed whitespace
##########
File path: spark2/src/main/java/org/apache/iceberg/spark/source/Reader.java
##########
@@ -330,7 +336,7 @@ public boolean enableBatchRead() {
boolean batchReadsEnabled = batchReadsEnabled(allParquetFileScanTasks, allOrcFileScanTasks);
this.readUsingBatch = batchReadsEnabled && hasNoDeleteFiles && (allOrcFileScanTasks ||
- (allParquetFileScanTasks && atLeastOneColumn && onlyPrimitives));
+ (allParquetFileScanTasks && atLeastOneColumn && onlyPrimitives));
Review comment:
removed unneeded whitespace
##########
File path: spark/src/test/java/org/apache/iceberg/spark/source/TestTimestampWithoutZone.java
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.iceberg.spark.source;
+
+import java.io.File;
+import java.io.IOException;
+import java.time.LocalDateTime;
+import java.util.List;
+import java.util.Locale;
+import java.util.UUID;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.AssertHelpers;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DataFiles;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.data.GenericAppenderFactory;
+import org.apache.iceberg.data.GenericRecord;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.hadoop.HadoopTables;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.spark.SparkUtil;
+import org.apache.iceberg.spark.data.GenericsHelpers;
+import org.apache.iceberg.types.Types;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.SaveMode;
+import org.apache.spark.sql.SparkSession;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import static org.apache.iceberg.Files.localOutput;
+
+@RunWith(Parameterized.class)
+public abstract class TestTimestampWithoutZone {
+ private static final Configuration CONF = new Configuration();
+ private static final HadoopTables TABLES = new HadoopTables(CONF);
+
+ private static final Schema SCHEMA = new Schema(
+ Types.NestedField.required(1, "id", Types.LongType.get()),
+ Types.NestedField.optional(2, "ts", Types.TimestampType.withoutZone()),
+ Types.NestedField.optional(3, "data", Types.StringType.get())
+ );
+
+ private static SparkSession spark = null;
+
+ @BeforeClass
+ public static void startSpark() {
+ TestTimestampWithoutZone.spark = SparkSession.builder().master("local[2]").getOrCreate();
+ }
+
+ @AfterClass
+ public static void stopSpark() {
+ SparkSession currentSpark = TestTimestampWithoutZone.spark;
+ TestTimestampWithoutZone.spark = null;
+ currentSpark.stop();
+ }
+
+ @Rule
+ public TemporaryFolder temp = new TemporaryFolder();
+
+ private final String format;
+ private final boolean vectorized;
+
+ @Parameterized.Parameters(name = "format = {0}, vectorized = {1}")
+ public static Object[][] parameters() {
+ return new Object[][] {
+ { "parquet", false },
+ { "parquet", true },
+ { "avro", false }
+ };
+ }
+
+ public TestTimestampWithoutZone(String format, boolean vectorized) {
+ this.format = format;
+ this.vectorized = vectorized;
+ }
+
+ private File parent = null;
+ private File unpartitioned = null;
+ private List<Record> records = null;
+
+ @Before
+ public void writeUnpartitionedTable() throws IOException {
+ this.parent = temp.newFolder("TestTimestampWithoutZone");
+ this.unpartitioned = new File(parent, "unpartitioned");
+ File dataFolder = new File(unpartitioned, "data");
+ Assert.assertTrue("Mkdir should succeed", dataFolder.mkdirs());
+
+ Table table = TABLES.create(SCHEMA, PartitionSpec.unpartitioned(), unpartitioned.toString());
+ Schema tableSchema = table.schema(); // use the table schema because ids are reassigned
+
+ FileFormat fileFormat = FileFormat.valueOf(format.toUpperCase(Locale.ENGLISH));
+
+ File testFile = new File(dataFolder, fileFormat.addExtension(UUID.randomUUID().toString()));
+
+ // create records using the table's schema
+ this.records = testRecords(tableSchema);
+
+ try (FileAppender<Record> writer = new GenericAppenderFactory(tableSchema).newAppender(
+ localOutput(testFile), fileFormat)) {
+ writer.addAll(records);
+ }
+
+ DataFile file = DataFiles.builder(PartitionSpec.unpartitioned())
+ .withRecordCount(records.size())
+ .withFileSizeInBytes(testFile.length())
+ .withPath(testFile.toString())
+ .build();
+
+ table.newAppend().appendFile(file).commit();
+ }
+
+ @Test
+ public void testUnpartitionedTimestampWithoutZone() {
+ assertEqualsSafe(SCHEMA.asStruct(), records, read(unpartitioned.toString(), vectorized));
+ }
+
+ @Test
+ public void testUnpartitionedTimestampWithoutZoneProjection() {
+ Schema projection = SCHEMA.select("id", "ts");
+ assertEqualsSafe(projection.asStruct(),
+ records.stream().map(r -> projectFlat(projection, r)).collect(Collectors.toList()),
+ read(unpartitioned.toString(), vectorized, "id", "ts"));
+ }
+
+ @Rule
+ public ExpectedException exception = ExpectedException.none();
+
+ @Test
+ public void testUnpartitionedTimestampWithoutZoneError() {
+ AssertHelpers.assertThrows(String.format("Read operation performed on a timestamp without timezone field while " +
+ "'%s' set to false should throw exception",
+ SparkUtil.HANDLE_TIMESTAMP_WITHOUT_TIMEZONE),
+ IllegalArgumentException.class, SparkUtil.TIMESTAMP_WITHOUT_TIMEZONE_ERROR, () -> spark.read()
+ .format("iceberg")
+ .option("vectorization-enabled", String.valueOf(vectorized))
+ .option(SparkUtil.HANDLE_TIMESTAMP_WITHOUT_TIMEZONE, "false")
+ .load(unpartitioned.toString())
+ .collectAsList());
+ }
+
+ @Test
+ public void testUnpartitionedTimestampWithoutZoneAppend() {
+ spark.read().format("iceberg")
+ .option(SparkUtil.HANDLE_TIMESTAMP_WITHOUT_TIMEZONE, "true")
+ .option("vectorization-enabled", String.valueOf(vectorized))
+ .load(unpartitioned.toString())
+
Review comment:
removed empty line
##########
File path: spark3/src/test/java/org/apache/iceberg/spark/sql/TestTimestampWithoutZone.java
##########
@@ -0,0 +1,193 @@
+/*
+ * 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.iceberg.spark.sql;
+
+import java.sql.Timestamp;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+import org.apache.iceberg.AssertHelpers;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.relocated.com.google.common.base.Joiner;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.spark.SparkCatalogTestBase;
+import org.apache.iceberg.spark.SparkSessionCatalog;
+import org.apache.iceberg.spark.SparkUtil;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.Types;
+import org.apache.spark.sql.util.CaseInsensitiveStringMap;
+import org.joda.time.DateTime;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runners.Parameterized;
+
+public class TestTimestampWithoutZone extends SparkCatalogTestBase {
+
+ private static final String newTableName = "created_table";
+ private final Map<String, String> config;
+
+ private static final Schema schema = new Schema(
+ Types.NestedField.required(1, "id", Types.LongType.get()),
+ Types.NestedField.required(2, "ts", Types.TimestampType.withoutZone()),
+ Types.NestedField.required(3, "tsz", Types.TimestampType.withZone())
+ );
+
+ private final List<Object[]> values = ImmutableList.of(
+ row(1L, toTimestamp("2021-01-01T00:00:00.0"), toTimestamp("2021-02-01T00:00:00.0")),
+ row(2L, toTimestamp("2021-01-01T00:00:00.0"), toTimestamp("2021-02-01T00:00:00.0")),
+ row(3L, toTimestamp("2021-01-01T00:00:00.0"), toTimestamp("2021-02-01T00:00:00.0"))
+ );
+
+ @Parameterized.Parameters(name = "catalogName = {0}, implementation = {1}, config = {2}")
+ public static Object[][] parameters() {
+ return new Object[][]{{"spark_catalog",
+ SparkSessionCatalog.class.getName(),
+ ImmutableMap.of(
+ "type", "hive",
+ "default-namespace", "default",
+ "parquet-enabled", "true",
+ "cache-enabled", "false"
+ )}
+ };
+ }
+
+ public TestTimestampWithoutZone(String catalogName, String implementation, Map<String, String> config) {
+ super(catalogName, implementation, config);
+ this.config = config;
+ }
+
+ @Before
+ public void createTables() {
+ spark.conf().set(SparkUtil.HANDLE_TIMESTAMP_WITHOUT_TIMEZONE, "false");
+ validationCatalog.createTable(tableIdent, schema);
+ }
+
+ @After
+ public void removeTables() {
+ validationCatalog.dropTable(tableIdent, true);
+ sql("DROP TABLE IF EXISTS %s", newTableName);
+ }
+
+ @Test
+ public void testWriteTimestampWithoutZoneError() {
+ String errorMessage = String.format("Write operation performed on a timestamp without timezone field while " +
+ "'%s' set to false should throw exception", SparkUtil.HANDLE_TIMESTAMP_WITHOUT_TIMEZONE);
+ Runnable insert = () -> sql("INSERT INTO %s VALUES %s", tableName, rowToSqlValues(values));
Review comment:
I have moved insert Runnable and errorMessage to the AssertThrows
I would like to leave `rowToSqlValues` if you do not mind. From my point of view this is better to have pure sql in this test
##########
File path: spark/src/test/java/org/apache/iceberg/spark/source/TestTimestampWithoutZone.java
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.iceberg.spark.source;
+
+import java.io.File;
+import java.io.IOException;
+import java.time.LocalDateTime;
+import java.util.List;
+import java.util.Locale;
+import java.util.UUID;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.AssertHelpers;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DataFiles;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.data.GenericAppenderFactory;
+import org.apache.iceberg.data.GenericRecord;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.hadoop.HadoopTables;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.spark.SparkUtil;
+import org.apache.iceberg.spark.data.GenericsHelpers;
+import org.apache.iceberg.types.Types;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.SaveMode;
+import org.apache.spark.sql.SparkSession;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import static org.apache.iceberg.Files.localOutput;
+
+@RunWith(Parameterized.class)
+public abstract class TestTimestampWithoutZone {
+ private static final Configuration CONF = new Configuration();
+ private static final HadoopTables TABLES = new HadoopTables(CONF);
+
+ private static final Schema SCHEMA = new Schema(
+ Types.NestedField.required(1, "id", Types.LongType.get()),
+ Types.NestedField.optional(2, "ts", Types.TimestampType.withoutZone()),
+ Types.NestedField.optional(3, "data", Types.StringType.get())
+ );
+
+ private static SparkSession spark = null;
+
+ @BeforeClass
+ public static void startSpark() {
+ TestTimestampWithoutZone.spark = SparkSession.builder().master("local[2]").getOrCreate();
+ }
+
+ @AfterClass
+ public static void stopSpark() {
+ SparkSession currentSpark = TestTimestampWithoutZone.spark;
+ TestTimestampWithoutZone.spark = null;
+ currentSpark.stop();
+ }
+
+ @Rule
+ public TemporaryFolder temp = new TemporaryFolder();
+
+ private final String format;
+ private final boolean vectorized;
+
+ @Parameterized.Parameters(name = "format = {0}, vectorized = {1}")
+ public static Object[][] parameters() {
+ return new Object[][] {
+ { "parquet", false },
+ { "parquet", true },
+ { "avro", false }
+ };
+ }
+
+ public TestTimestampWithoutZone(String format, boolean vectorized) {
+ this.format = format;
+ this.vectorized = vectorized;
+ }
+
+ private File parent = null;
+ private File unpartitioned = null;
+ private List<Record> records = null;
+
+ @Before
+ public void writeUnpartitionedTable() throws IOException {
+ this.parent = temp.newFolder("TestTimestampWithoutZone");
+ this.unpartitioned = new File(parent, "unpartitioned");
+ File dataFolder = new File(unpartitioned, "data");
+ Assert.assertTrue("Mkdir should succeed", dataFolder.mkdirs());
+
+ Table table = TABLES.create(SCHEMA, PartitionSpec.unpartitioned(), unpartitioned.toString());
+ Schema tableSchema = table.schema(); // use the table schema because ids are reassigned
+
+ FileFormat fileFormat = FileFormat.valueOf(format.toUpperCase(Locale.ENGLISH));
+
+ File testFile = new File(dataFolder, fileFormat.addExtension(UUID.randomUUID().toString()));
+
+ // create records using the table's schema
+ this.records = testRecords(tableSchema);
+
+ try (FileAppender<Record> writer = new GenericAppenderFactory(tableSchema).newAppender(
+ localOutput(testFile), fileFormat)) {
+ writer.addAll(records);
+ }
+
+ DataFile file = DataFiles.builder(PartitionSpec.unpartitioned())
+ .withRecordCount(records.size())
+ .withFileSizeInBytes(testFile.length())
+ .withPath(testFile.toString())
+ .build();
+
+ table.newAppend().appendFile(file).commit();
+ }
+
+ @Test
+ public void testUnpartitionedTimestampWithoutZone() {
+ assertEqualsSafe(SCHEMA.asStruct(), records, read(unpartitioned.toString(), vectorized));
+ }
+
+ @Test
+ public void testUnpartitionedTimestampWithoutZoneProjection() {
+ Schema projection = SCHEMA.select("id", "ts");
+ assertEqualsSafe(projection.asStruct(),
+ records.stream().map(r -> projectFlat(projection, r)).collect(Collectors.toList()),
+ read(unpartitioned.toString(), vectorized, "id", "ts"));
+ }
+
+ @Rule
+ public ExpectedException exception = ExpectedException.none();
+
+ @Test
+ public void testUnpartitionedTimestampWithoutZoneError() {
+ AssertHelpers.assertThrows(String.format("Read operation performed on a timestamp without timezone field while " +
+ "'%s' set to false should throw exception",
+ SparkUtil.HANDLE_TIMESTAMP_WITHOUT_TIMEZONE),
+ IllegalArgumentException.class,
+ SparkUtil.TIMESTAMP_WITHOUT_TIMEZONE_ERROR,
+ () -> spark.read().format("iceberg")
Review comment:
you are right, fixed
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r670224086
##########
File path: spark/src/test/java/org/apache/iceberg/spark/data/AvroDataTest.java
##########
@@ -185,4 +190,51 @@ public void testMixedTypes() throws IOException {
writeAndValidate(schema);
}
+
+ @Test
+ public void testTimestampWithoutZone() throws IOException {
+ withSQLConf(ImmutableMap.of(SparkUtil.HANDLE_TIMESTAMP_WITHOUT_TIMEZONE, "true"), () -> {
+ Schema schema = TypeUtil.assignIncreasingFreshIds(new Schema(
+ required(0, "id", LongType.get()),
+ optional(1, "ts_without_zone", Types.TimestampType.withoutZone())));
+
+ writeAndValidate(schema);
+ });
+ }
+
+ protected void withSQLConf(Map<String, String> conf, Action action) throws IOException {
+ SQLConf sqlConf = SQLConf.get();
+
+ Map<String, String> currentConfValues = Maps.newHashMap();
+ conf.keySet().forEach(confKey -> {
+ if (sqlConf.contains(confKey)) {
+ String currentConfValue = sqlConf.getConfString(confKey);
+ currentConfValues.put(confKey, currentConfValue);
+ }
+ });
+
+ conf.forEach((confKey, confValue) -> {
+ if (SQLConf.staticConfKeys().contains(confKey)) {
+ throw new RuntimeException("Cannot modify the value of a static config: " + confKey);
+ }
+ sqlConf.setConfString(confKey, confValue);
+ });
+
+ try {
+ action.invoke();
+ } finally {
+ conf.forEach((confKey, confValue) -> {
+ if (currentConfValues.containsKey(confKey)) {
+ sqlConf.setConfString(confKey, currentConfValues.get(confKey));
+ } else {
+ sqlConf.unsetConf(confKey);
+ }
+ });
+ }
+ }
+
+ @FunctionalInterface
+ protected interface Action {
+ void invoke() throws IOException;
+ }
Review comment:
I am fully agree with you, it can be moved to separate interface with static method and placed in some general package like
```java
@FunctionalInterface
public interface ConfigurableTestSQLConf {
void invoke() throws IOException;
static void withSQLConf(Map<String, String> conf, ConfigurableTestSQLConf action) throws IOException {
SQLConf sqlConf = SQLConf.get();
Map<String, String> currentConfValues = Maps.newHashMap();
conf.keySet().forEach(confKey -> {
if (sqlConf.contains(confKey)) {
String currentConfValue = sqlConf.getConfString(confKey);
currentConfValues.put(confKey, currentConfValue);
}
});
conf.forEach((confKey, confValue) -> {
if (SQLConf.staticConfKeys().contains(confKey)) {
throw new RuntimeException("Cannot modify the value of a static config: " + confKey);
}
sqlConf.setConfString(confKey, confValue);
});
try {
action.invoke();
} finally {
conf.forEach((confKey, confValue) -> {
if (currentConfValues.containsKey(confKey)) {
sqlConf.setConfString(confKey, currentConfValues.get(confKey));
} else {
sqlConf.unsetConf(confKey);
}
});
}
}
}
```
But this part better to do in separate PR, because other packages will be affected
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r662038363
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkUtil.java
##########
@@ -20,21 +20,38 @@
package org.apache.iceberg.spark;
import java.util.List;
+import java.util.Map;
import java.util.function.BiFunction;
import java.util.function.Function;
import java.util.stream.Collectors;
import org.apache.iceberg.PartitionField;
import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
import org.apache.iceberg.Table;
import org.apache.iceberg.hadoop.HadoopFileIO;
import org.apache.iceberg.io.FileIO;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.transforms.Transform;
import org.apache.iceberg.transforms.UnknownTransform;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.Pair;
+import org.apache.spark.sql.RuntimeConfig;
import org.apache.spark.util.SerializableConfiguration;
public class SparkUtil {
+
+ public static final String HANDLE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY =
+ "spark.sql.iceberg.convert-timestamp-without-timezone";
+ public static final String TIMESTAMP_WITHOUT_TIMEZONE_ERROR = String.format("Cannot handle timestamp without" +
+ " timezone fields in Spark. Spark does not natively support this type but if you would like to handle all" +
+ " timestamps as timestamp with timezone set '%s' to true. This will not change the underlying values stored" +
+ " but will change their displayed values in Spark. For more information please see" +
+ " https://docs.databricks.com/spark/latest/dataframes-datasets/dates-timestamps.html#ansi-sql-and" +
+ "-spark-sql-timestamps", HANDLE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY);
+ public static final String READ_TIMESTAMP_AS_TIMESTAMP_WITHOUT_TIMEZONE =
Review comment:
Yeah, you are right.
`HANDLE_TIMESTAMP_WITHOUT_TIMEZONE` is responsible for allowing reading and writing of timestamp types without timezone.
`USE_TIMESTAMP_WITHOUT_TIME_ZONE_IN_NEW_TABLES` is responsible for selecting which type will be used in new tables `Types.TimestampType#withoutZone()` or `Types.TimestampType#withZone()`
I changed variable names, looks more clear now from my point of view, but please let me know if you want to change names to something else
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r670206791
##########
File path: spark2/src/main/java/org/apache/iceberg/spark/source/Reader.java
##########
@@ -183,6 +187,8 @@ private Expression filterExpression() {
private StructType lazyType() {
if (type == null) {
+ Preconditions.checkArgument(readTimestampWithoutZone || !SparkUtil.hasTimestampWithoutZone(lazySchema()),
+ SparkUtil.TIMESTAMP_WITHOUT_TIMEZONE_ERROR);
Review comment:
For reference, this is something just to consider and not a strong mandate or requirement to merge.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer merged pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer merged pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r660732193
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkFixupTimestampType.java
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.iceberg.spark;
+
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.types.FixupTypes;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
+
+/**
+ * By default Spark TimestampType should be converted to Types.TimestampType.withZone() iceberg type.
+ * But we also can convert TimestampType to Types.TimestampType.withoutZone() iceberg type
+ * by setting SparkUtil.STORE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY to 'true'
+ */
+class SparkFixupTimestampType extends FixupTypes {
+
+ private final boolean useTimestampWithoutZone;
+
+ private SparkFixupTimestampType(Schema referenceSchema, boolean useTimestampWithoutZone) {
Review comment:
actually do we need to gate this at all? We won't allow calling the conversion code on a TS so it may just be fine to always use this time conversion and just add it to SparkFixup?
Ie
If handleTimestampWithouSession is false, we should fail in IcebergSource right?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r664647453
##########
File path: spark/src/test/java/org/apache/iceberg/spark/source/TestTimestampWithoutZone.java
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.iceberg.spark.source;
+
+import java.io.File;
+import java.io.IOException;
+import java.time.LocalDateTime;
+import java.util.List;
+import java.util.Locale;
+import java.util.UUID;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.AssertHelpers;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DataFiles;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.data.GenericAppenderFactory;
+import org.apache.iceberg.data.GenericRecord;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.hadoop.HadoopTables;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.spark.SparkUtil;
+import org.apache.iceberg.spark.data.GenericsHelpers;
+import org.apache.iceberg.types.Types;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.SaveMode;
+import org.apache.spark.sql.SparkSession;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import static org.apache.iceberg.Files.localOutput;
+
+@RunWith(Parameterized.class)
+public abstract class TestTimestampWithoutZone {
+ private static final Configuration CONF = new Configuration();
+ private static final HadoopTables TABLES = new HadoopTables(CONF);
+
+ private static final Schema SCHEMA = new Schema(
+ Types.NestedField.required(1, "id", Types.LongType.get()),
+ Types.NestedField.optional(2, "ts", Types.TimestampType.withoutZone()),
+ Types.NestedField.optional(3, "data", Types.StringType.get())
+ );
+
+ private static SparkSession spark = null;
+
+ @BeforeClass
+ public static void startSpark() {
+ TestTimestampWithoutZone.spark = SparkSession.builder().master("local[2]").getOrCreate();
+ }
+
+ @AfterClass
+ public static void stopSpark() {
+ SparkSession currentSpark = TestTimestampWithoutZone.spark;
+ TestTimestampWithoutZone.spark = null;
+ currentSpark.stop();
+ }
+
+ @Rule
+ public TemporaryFolder temp = new TemporaryFolder();
+
+ private final String format;
+ private final boolean vectorized;
+
+ @Parameterized.Parameters(name = "format = {0}, vectorized = {1}")
+ public static Object[][] parameters() {
+ return new Object[][] {
+ { "parquet", false },
+ { "parquet", true },
+ { "avro", false }
+ };
+ }
+
+ public TestTimestampWithoutZone(String format, boolean vectorized) {
+ this.format = format;
+ this.vectorized = vectorized;
+ }
+
+ private File parent = null;
+ private File unpartitioned = null;
+ private List<Record> records = null;
+
+ @Before
+ public void writeUnpartitionedTable() throws IOException {
+ this.parent = temp.newFolder("TestTimestampWithoutZone");
+ this.unpartitioned = new File(parent, "unpartitioned");
+ File dataFolder = new File(unpartitioned, "data");
+ Assert.assertTrue("Mkdir should succeed", dataFolder.mkdirs());
+
+ Table table = TABLES.create(SCHEMA, PartitionSpec.unpartitioned(), unpartitioned.toString());
+ Schema tableSchema = table.schema(); // use the table schema because ids are reassigned
+
+ FileFormat fileFormat = FileFormat.valueOf(format.toUpperCase(Locale.ENGLISH));
+
+ File testFile = new File(dataFolder, fileFormat.addExtension(UUID.randomUUID().toString()));
+
+ // create records using the table's schema
+ this.records = testRecords(tableSchema);
+
+ try (FileAppender<Record> writer = new GenericAppenderFactory(tableSchema).newAppender(
+ localOutput(testFile), fileFormat)) {
+ writer.addAll(records);
+ }
+
+ DataFile file = DataFiles.builder(PartitionSpec.unpartitioned())
+ .withRecordCount(records.size())
+ .withFileSizeInBytes(testFile.length())
+ .withPath(testFile.toString())
+ .build();
+
+ table.newAppend().appendFile(file).commit();
+ }
+
+ @Test
+ public void testUnpartitionedTimestampWithoutZone() {
+ assertEqualsSafe(SCHEMA.asStruct(), records, read(unpartitioned.toString(), vectorized));
+ }
+
+ @Test
+ public void testUnpartitionedTimestampWithoutZoneProjection() {
+ Schema projection = SCHEMA.select("id", "ts");
+ assertEqualsSafe(projection.asStruct(),
+ records.stream().map(r -> projectFlat(projection, r)).collect(Collectors.toList()),
+ read(unpartitioned.toString(), vectorized, "id", "ts"));
+ }
+
+ @Rule
+ public ExpectedException exception = ExpectedException.none();
+
+ @Test
+ public void testUnpartitionedTimestampWithoutZoneError() {
+ AssertHelpers.assertThrows(String.format("Read operation performed on a timestamp without timezone field while " +
+ "'%s' set to false should throw exception",
+ SparkUtil.HANDLE_TIMESTAMP_WITHOUT_TIMEZONE),
+ IllegalArgumentException.class, SparkUtil.TIMESTAMP_WITHOUT_TIMEZONE_ERROR, () -> spark.read()
+ .format("iceberg")
+ .option("vectorization-enabled", String.valueOf(vectorized))
+ .option(SparkUtil.HANDLE_TIMESTAMP_WITHOUT_TIMEZONE, "false")
+ .load(unpartitioned.toString())
+ .collectAsList());
+ }
+
+ @Test
+ public void testUnpartitionedTimestampWithoutZoneAppend() {
+ spark.read().format("iceberg")
+ .option(SparkUtil.HANDLE_TIMESTAMP_WITHOUT_TIMEZONE, "true")
+ .option("vectorization-enabled", String.valueOf(vectorized))
+ .load(unpartitioned.toString())
+
Review comment:
nit: Empty Line
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r660683810
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkFixupTimestampType.java
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.iceberg.spark;
+
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.types.FixupTypes;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
+
+/**
+ * By default Spark TimestampType should be converted to Types.TimestampType.withZone() iceberg type.
+ * But we also can convert TimestampType to Types.TimestampType.withoutZone() iceberg type
+ * by setting SparkUtil.STORE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY to 'true'
Review comment:
Using a {@link class#parameter} thing here would keep this in sync with the variable name
Same note for referring to for time stamp types
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r667880407
##########
File path: spark/src/test/java/org/apache/iceberg/spark/source/TestTimestampWithoutZone.java
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.iceberg.spark.source;
+
+import java.io.File;
+import java.io.IOException;
+import java.time.LocalDateTime;
+import java.util.List;
+import java.util.Locale;
+import java.util.UUID;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.AssertHelpers;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DataFiles;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.data.GenericAppenderFactory;
+import org.apache.iceberg.data.GenericRecord;
+import org.apache.iceberg.data.Record;
+import org.apache.iceberg.hadoop.HadoopTables;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.spark.SparkUtil;
+import org.apache.iceberg.spark.data.GenericsHelpers;
+import org.apache.iceberg.types.Types;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.SaveMode;
+import org.apache.spark.sql.SparkSession;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import static org.apache.iceberg.Files.localOutput;
+
+@RunWith(Parameterized.class)
+public abstract class TestTimestampWithoutZone {
+ private static final Configuration CONF = new Configuration();
+ private static final HadoopTables TABLES = new HadoopTables(CONF);
+
+ private static final Schema SCHEMA = new Schema(
+ Types.NestedField.required(1, "id", Types.LongType.get()),
+ Types.NestedField.optional(2, "ts", Types.TimestampType.withoutZone()),
+ Types.NestedField.optional(3, "data", Types.StringType.get())
+ );
+
+ private static SparkSession spark = null;
+
+ @BeforeClass
+ public static void startSpark() {
+ TestTimestampWithoutZone.spark = SparkSession.builder().master("local[2]").getOrCreate();
+ }
+
+ @AfterClass
+ public static void stopSpark() {
+ SparkSession currentSpark = TestTimestampWithoutZone.spark;
+ TestTimestampWithoutZone.spark = null;
+ currentSpark.stop();
+ }
+
+ @Rule
+ public TemporaryFolder temp = new TemporaryFolder();
+
+ private final String format;
+ private final boolean vectorized;
+
+ @Parameterized.Parameters(name = "format = {0}, vectorized = {1}")
+ public static Object[][] parameters() {
+ return new Object[][] {
+ { "parquet", false },
+ { "parquet", true },
+ { "avro", false }
+ };
+ }
+
+ public TestTimestampWithoutZone(String format, boolean vectorized) {
+ this.format = format;
+ this.vectorized = vectorized;
+ }
+
+ private File parent = null;
+ private File unpartitioned = null;
+ private List<Record> records = null;
+
+ @Before
+ public void writeUnpartitionedTable() throws IOException {
+ this.parent = temp.newFolder("TestTimestampWithoutZone");
+ this.unpartitioned = new File(parent, "unpartitioned");
+ File dataFolder = new File(unpartitioned, "data");
+ Assert.assertTrue("Mkdir should succeed", dataFolder.mkdirs());
+
+ Table table = TABLES.create(SCHEMA, PartitionSpec.unpartitioned(), unpartitioned.toString());
+ Schema tableSchema = table.schema(); // use the table schema because ids are reassigned
+
+ FileFormat fileFormat = FileFormat.valueOf(format.toUpperCase(Locale.ENGLISH));
+
+ File testFile = new File(dataFolder, fileFormat.addExtension(UUID.randomUUID().toString()));
+
+ // create records using the table's schema
+ this.records = testRecords(tableSchema);
+
+ try (FileAppender<Record> writer = new GenericAppenderFactory(tableSchema).newAppender(
+ localOutput(testFile), fileFormat)) {
+ writer.addAll(records);
+ }
+
+ DataFile file = DataFiles.builder(PartitionSpec.unpartitioned())
+ .withRecordCount(records.size())
+ .withFileSizeInBytes(testFile.length())
+ .withPath(testFile.toString())
+ .build();
+
+ table.newAppend().appendFile(file).commit();
+ }
+
+ @Test
+ public void testUnpartitionedTimestampWithoutZone() {
+ assertEqualsSafe(SCHEMA.asStruct(), records, read(unpartitioned.toString(), vectorized));
+ }
+
+ @Test
+ public void testUnpartitionedTimestampWithoutZoneProjection() {
+ Schema projection = SCHEMA.select("id", "ts");
+ assertEqualsSafe(projection.asStruct(),
+ records.stream().map(r -> projectFlat(projection, r)).collect(Collectors.toList()),
+ read(unpartitioned.toString(), vectorized, "id", "ts"));
+ }
+
+ @Rule
+ public ExpectedException exception = ExpectedException.none();
+
+ @Test
+ public void testUnpartitionedTimestampWithoutZoneError() {
+ AssertHelpers.assertThrows(String.format("Read operation performed on a timestamp without timezone field while " +
+ "'%s' set to false should throw exception",
+ SparkUtil.HANDLE_TIMESTAMP_WITHOUT_TIMEZONE),
+ IllegalArgumentException.class,
+ SparkUtil.TIMESTAMP_WITHOUT_TIMEZONE_ERROR,
+ () -> spark.read().format("iceberg")
Review comment:
wrong indentation here, .option should be indented as it's a continuation of spark.read.format and not another arg to assertThrows
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r660687382
##########
File path: spark/src/main/java/org/apache/iceberg/spark/TypeToSparkType.java
##########
@@ -104,12 +104,14 @@ public DataType primitive(Type.PrimitiveType primitive) {
throw new UnsupportedOperationException(
"Spark does not support time fields");
case TIMESTAMP:
- Types.TimestampType timestamp = (Types.TimestampType) primitive;
- if (timestamp.shouldAdjustToUTC()) {
- return TimestampType$.MODULE$;
- }
- throw new UnsupportedOperationException(
- "Spark does not support timestamp without time zone fields");
+// Types.TimestampType timestamp = (Types.TimestampType) primitive;
Review comment:
Remove commented block
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r660684452
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkFixupTimestampType.java
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.iceberg.spark;
+
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.types.FixupTypes;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
+
+/**
+ * By default Spark TimestampType should be converted to Types.TimestampType.withZone() iceberg type.
+ * But we also can convert TimestampType to Types.TimestampType.withoutZone() iceberg type
+ * by setting SparkUtil.STORE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY to 'true'
+ */
+class SparkFixupTimestampType extends FixupTypes {
+
+ private final boolean useTimestampWithoutZone;
+
+ private SparkFixupTimestampType(Schema referenceSchema, boolean useTimestampWithoutZone) {
Review comment:
ie in the parent "If useTimestampWIthoutTimeZone THEN use SparkFixupTimestampType else don't"
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r661537493
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkUtil.java
##########
@@ -100,4 +117,55 @@ public static void validatePartitionTransforms(PartitionSpec spec) {
}
}
}
+
+ /**
+ * Responsible for checking if the table schema has a timestamp without timezone column
+ * @param schema table schema to check if it contains a timestamp without timezone column
+ * @return boolean indicating if the schema passed in has a timestamp field without a timezone
+ */
+ public static boolean hasTimestampWithoutZone(Schema schema) {
+ return TypeUtil.find(schema, t -> Types.TimestampType.withoutZone().equals(t)) != null;
+ }
+
+ /**
+ * Allow reading timestamp without time zone as timestamp with time zone. Generally, this is not safe as timestamp
+ * without time zone is supposed to represent wall clock time semantics, i.e. no matter the reader/writer timezone
+ * 3PM should always be read as 3PM, but timestamp with time zone represents instant semantics, i.e the timestamp
+ * is adjusted so that the corresponding time in the reader timezone is displayed.
+ * When set to false (default), we throw an exception at runtime
+ * "Spark does not support timestamp without time zone fields" if reading timestamp without time zone fields
+ *
+ * @param readerConfig table read options
+ * @param sessionConf spark session configurations
+ * @return boolean indicating if reading timestamps without timezone is allowed
+ */
+ public static boolean canHandleTimestampWithoutZone(Map<String, String> readerConfig, RuntimeConfig sessionConf) {
+ String readerOption = readerConfig.get(HANDLE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY);
+ if (readerOption != null) {
+ return Boolean.parseBoolean(readerOption);
+ }
+ String sessionConfValue = sessionConf.get(HANDLE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY, null);
+ if (sessionConfValue != null) {
+ return Boolean.parseBoolean(sessionConfValue);
+ }
+ return false;
+ }
+
+ /**
+ * Check whether the spark session config contains a 'spark.sql.iceberg.read-timestamp-as-timestamp-without-timezone'
+ * property.
+ * Default value - false
+ *
+ * @param sessionConf a spark runtime config
+ * @return true if the session config has spark.sql.iceberg.read-timestamp-as-timestamp-without-timezone property
Review comment:
Java doc link to property {@link property} here and above ^
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r661507384
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkFixupTimestampType.java
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.iceberg.spark;
+
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.types.FixupTypes;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
+
+/**
+ * By default Spark TimestampType should be converted to Types.TimestampType.withZone() iceberg type.
+ * But we also can convert TimestampType to Types.TimestampType.withoutZone() iceberg type
+ * by setting SparkUtil.STORE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY to 'true'
+ */
+class SparkFixupTimestampType extends FixupTypes {
+
+ private final boolean useTimestampWithoutZone;
+
+ private SparkFixupTimestampType(Schema referenceSchema, boolean useTimestampWithoutZone) {
Review comment:
We need this class because this TS conversion will be used only when we creating new table from spark, in this case we will be able to select which timestamp type will be used `Types.TimestampType.withZone()` or `Types.TimestampType.withoutZone()` by [READ_TIMESTAMP_AS_TIMESTAMP_WITHOUT_TIMEZONE](https://github.com/apache/iceberg/blob/f2fdf2a5f9ffc64b44b353e9fd7d09ed21f8c416/spark/src/main/java/org/apache/iceberg/spark/SparkUtil.java#L52) session property
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r662039753
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkFixupTimestampType.java
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.iceberg.spark;
+
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.types.FixupTypes;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
+
+/**
+ * By default Spark TimestampType should be converted to Types.TimestampType.withZone() iceberg type.
+ * But we also can convert TimestampType to Types.TimestampType.withoutZone() iceberg type
+ * by setting SparkUtil.STORE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY to 'true'
+ */
+class SparkFixupTimestampType extends FixupTypes {
+
+ private final boolean useTimestampWithoutZone;
+
+ private SparkFixupTimestampType(Schema referenceSchema, boolean useTimestampWithoutZone) {
Review comment:
we have tests for [both parameters](https://github.com/apache/iceberg/pull/2757/files/f2fdf2a5f9ffc64b44b353e9fd7d09ed21f8c416#diff-5735c57982a67ce763ffa1c7831f6d107751c44e5be3b07f288bd32c02510240R134)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r660708172
##########
File path: spark/src/test/java/org/apache/iceberg/spark/data/AvroDataTest.java
##########
@@ -40,6 +42,8 @@
protected abstract void writeAndValidate(Schema schema) throws IOException;
+ protected abstract SparkSession getSparkSession();
Review comment:
Like I see we use SparkSession.builder().master("local[2]").getOrCreate(); in a bunch of places, we could just switch that to a utility method that always sets the handle TZ prop
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#issuecomment-880868777
Thanks @sshkvar + @bkahloon ! This is a great contribution
Solves #2244
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r670191691
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkFixupTimestampType.java
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.iceberg.spark;
+
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.types.FixupTypes;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
+
+/**
+ * By default Spark type {@link org.apache.iceberg.types.Types.TimestampType} should be converted to
+ * {@link Types.TimestampType#withZone()} iceberg type. But we also can convert
+ * {@link org.apache.iceberg.types.Types.TimestampType} to {@link Types.TimestampType#withoutZone()} iceberg type
+ * by setting {@link SparkUtil#USE_TIMESTAMP_WITHOUT_TIME_ZONE_IN_NEW_TABLES} to 'true'
+ */
+class SparkFixupTimestampType extends FixupTypes {
+
+ private SparkFixupTimestampType(Schema referenceSchema) {
+ super(referenceSchema);
+ }
+
+ static Schema fixup(Schema schema) {
+ return new Schema(TypeUtil.visit(schema,
+ new SparkFixupTimestampType(schema)).asStructType().fields());
Review comment:
Nit: I think this is overly indented.
When continuing an expression on a second line, we usually indent 4 spaces (as opposed to the normal 2 spaces for changes in scope etc).
So I think that `new SparkFixup…` on this line should align the n of new with the u from return above it (4 spaces in from the start of the word `return`).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#issuecomment-878176765
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r660704487
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkUtil.java
##########
@@ -20,21 +20,38 @@
package org.apache.iceberg.spark;
import java.util.List;
+import java.util.Map;
import java.util.function.BiFunction;
import java.util.function.Function;
import java.util.stream.Collectors;
import org.apache.iceberg.PartitionField;
import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
import org.apache.iceberg.Table;
import org.apache.iceberg.hadoop.HadoopFileIO;
import org.apache.iceberg.io.FileIO;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.transforms.Transform;
import org.apache.iceberg.transforms.UnknownTransform;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.Pair;
+import org.apache.spark.sql.RuntimeConfig;
import org.apache.spark.util.SerializableConfiguration;
public class SparkUtil {
+
+ public static final String HANDLE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY =
+ "spark.sql.iceberg.convert-timestamp-without-timezone";
+ public static final String TIMESTAMP_WITHOUT_TIMEZONE_ERROR = String.format("Cannot handle timestamp without" +
+ " timezone fields in Spark. Spark does not natively support this type but if you would like to handle all" +
+ " timestamps as timestamp with timezone set '%s' to true. This will not change the underlying values stored" +
+ " but will change their displayed values in Spark. For more information please see" +
+ " https://docs.databricks.com/spark/latest/dataframes-datasets/dates-timestamps.html#ansi-sql-and" +
+ "-spark-sql-timestamps", HANDLE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY);
+ public static final String READ_TIMESTAMP_AS_TIMESTAMP_WITHOUT_TIMEZONE =
Review comment:
we have 1 property which responsible for reading and writing `HANDLE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY `
this property `READ_TIMESTAMP_AS_TIMESTAMP_WITHOUT_TIMEZONE` is responsible for converting spark `Timestamp` type to iceberg type. Depends on this property we will have `Types.TimestampType.withZone()` or `Types.TimestampType.withoutZone()`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r664648361
##########
File path: spark2/src/main/java/org/apache/iceberg/spark/source/Reader.java
##########
@@ -330,7 +336,7 @@ public boolean enableBatchRead() {
boolean batchReadsEnabled = batchReadsEnabled(allParquetFileScanTasks, allOrcFileScanTasks);
this.readUsingBatch = batchReadsEnabled && hasNoDeleteFiles && (allOrcFileScanTasks ||
- (allParquetFileScanTasks && atLeastOneColumn && onlyPrimitives));
+ (allParquetFileScanTasks && atLeastOneColumn && onlyPrimitives));
Review comment:
nit: Whitespace change unneeded
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r664655115
##########
File path: spark3/src/test/java/org/apache/iceberg/spark/sql/TestTimestampWithoutZone.java
##########
@@ -0,0 +1,193 @@
+/*
+ * 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.iceberg.spark.sql;
+
+import java.sql.Timestamp;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+import org.apache.iceberg.AssertHelpers;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.relocated.com.google.common.base.Joiner;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.spark.SparkCatalogTestBase;
+import org.apache.iceberg.spark.SparkSessionCatalog;
+import org.apache.iceberg.spark.SparkUtil;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.Types;
+import org.apache.spark.sql.util.CaseInsensitiveStringMap;
+import org.joda.time.DateTime;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runners.Parameterized;
+
+public class TestTimestampWithoutZone extends SparkCatalogTestBase {
+
+ private static final String newTableName = "created_table";
+ private final Map<String, String> config;
+
+ private static final Schema schema = new Schema(
+ Types.NestedField.required(1, "id", Types.LongType.get()),
+ Types.NestedField.required(2, "ts", Types.TimestampType.withoutZone()),
+ Types.NestedField.required(3, "tsz", Types.TimestampType.withZone())
+ );
+
+ private final List<Object[]> values = ImmutableList.of(
+ row(1L, toTimestamp("2021-01-01T00:00:00.0"), toTimestamp("2021-02-01T00:00:00.0")),
+ row(2L, toTimestamp("2021-01-01T00:00:00.0"), toTimestamp("2021-02-01T00:00:00.0")),
+ row(3L, toTimestamp("2021-01-01T00:00:00.0"), toTimestamp("2021-02-01T00:00:00.0"))
+ );
+
+ @Parameterized.Parameters(name = "catalogName = {0}, implementation = {1}, config = {2}")
+ public static Object[][] parameters() {
+ return new Object[][]{{"spark_catalog",
+ SparkSessionCatalog.class.getName(),
+ ImmutableMap.of(
+ "type", "hive",
+ "default-namespace", "default",
+ "parquet-enabled", "true",
+ "cache-enabled", "false"
+ )}
+ };
+ }
+
+ public TestTimestampWithoutZone(String catalogName, String implementation, Map<String, String> config) {
+ super(catalogName, implementation, config);
+ this.config = config;
+ }
+
+ @Before
+ public void createTables() {
+ spark.conf().set(SparkUtil.HANDLE_TIMESTAMP_WITHOUT_TIMEZONE, "false");
+ validationCatalog.createTable(tableIdent, schema);
+ }
+
+ @After
+ public void removeTables() {
+ validationCatalog.dropTable(tableIdent, true);
+ sql("DROP TABLE IF EXISTS %s", newTableName);
+ }
+
+ @Test
+ public void testWriteTimestampWithoutZoneError() {
+ String errorMessage = String.format("Write operation performed on a timestamp without timezone field while " +
+ "'%s' set to false should throw exception", SparkUtil.HANDLE_TIMESTAMP_WITHOUT_TIMEZONE);
+ Runnable insert = () -> sql("INSERT INTO %s VALUES %s", tableName, rowToSqlValues(values));
Review comment:
Instead of using the rowToSqlValues function, it may be simpler to just do a spark.createDataset(values).write command. That way we don't have to worry about string building
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r668337753
##########
File path: spark/src/test/java/org/apache/iceberg/spark/data/AvroDataTest.java
##########
@@ -185,4 +188,18 @@ public void testMixedTypes() throws IOException {
writeAndValidate(schema);
}
+
+ @Test
+ public void testTimestampWithoutZone() throws IOException {
+ Option<SparkSession> sparkSession = SparkSession.getActiveSession();
+ if (sparkSession.isDefined()) {
+ sparkSession.get().conf().set(SparkUtil.HANDLE_TIMESTAMP_WITHOUT_TIMEZONE, "true");
+ }
Review comment:
This is in `AvroDataTest`, but there is some code for handling additional Spark SQL conf options here that might be something worth copying:
https://github.com/apache/iceberg/blob/25eaebacbd1e250d2d884b49fc753a23b6aa6eaf/spark/src/test/java/org/apache/iceberg/spark/SparkTestBase.java#L171-L206
Additionally, it might be good to unset this property after its use (so the configuration doesn't get mixed up with other tests now or in the future).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r661537805
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkUtil.java
##########
@@ -100,4 +117,55 @@ public static void validatePartitionTransforms(PartitionSpec spec) {
}
}
}
+
+ /**
+ * Responsible for checking if the table schema has a timestamp without timezone column
+ * @param schema table schema to check if it contains a timestamp without timezone column
+ * @return boolean indicating if the schema passed in has a timestamp field without a timezone
+ */
+ public static boolean hasTimestampWithoutZone(Schema schema) {
+ return TypeUtil.find(schema, t -> Types.TimestampType.withoutZone().equals(t)) != null;
+ }
+
+ /**
+ * Allow reading timestamp without time zone as timestamp with time zone. Generally, this is not safe as timestamp
+ * without time zone is supposed to represent wall clock time semantics, i.e. no matter the reader/writer timezone
+ * 3PM should always be read as 3PM, but timestamp with time zone represents instant semantics, i.e the timestamp
+ * is adjusted so that the corresponding time in the reader timezone is displayed.
+ * When set to false (default), we throw an exception at runtime
+ * "Spark does not support timestamp without time zone fields" if reading timestamp without time zone fields
+ *
+ * @param readerConfig table read options
+ * @param sessionConf spark session configurations
+ * @return boolean indicating if reading timestamps without timezone is allowed
+ */
+ public static boolean canHandleTimestampWithoutZone(Map<String, String> readerConfig, RuntimeConfig sessionConf) {
+ String readerOption = readerConfig.get(HANDLE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY);
+ if (readerOption != null) {
+ return Boolean.parseBoolean(readerOption);
+ }
+ String sessionConfValue = sessionConf.get(HANDLE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY, null);
+ if (sessionConfValue != null) {
+ return Boolean.parseBoolean(sessionConfValue);
+ }
+ return false;
+ }
+
+ /**
+ * Check whether the spark session config contains a 'spark.sql.iceberg.read-timestamp-as-timestamp-without-timezone'
+ * property.
+ * Default value - false
+ *
+ * @param sessionConf a spark runtime config
+ * @return true if the session config has spark.sql.iceberg.read-timestamp-as-timestamp-without-timezone property
+ * and this property is set to true
+ */
+ public static boolean shouldStoreTimestampWithoutZone(RuntimeConfig sessionConf) {
+ String sessionConfValue = sessionConf.get(READ_TIMESTAMP_AS_TIMESTAMP_WITHOUT_TIMEZONE, null);
Review comment:
Is this the right property? I thought this is the "handle" one
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r661567307
##########
File path: spark3/src/main/java/org/apache/iceberg/spark/source/SparkBatchScan.java
##########
@@ -158,8 +166,10 @@ public PartitionReaderFactory createReaderFactory() {
boolean batchReadsEnabled = batchReadsEnabled(allParquetFileScanTasks, allOrcFileScanTasks);
+ boolean hasTimestampWithoutZone = SparkUtil.hasTimestampWithoutZone(expectedSchema);
+
boolean readUsingBatch = batchReadsEnabled && hasNoDeleteFiles && (allOrcFileScanTasks ||
- (allParquetFileScanTasks && atLeastOneColumn && onlyPrimitives));
+ (allParquetFileScanTasks && atLeastOneColumn && onlyPrimitives && !hasTimestampWithoutZone));
Review comment:
We don't need the !hasTimestampWithoutZone here right? for batch reading? Or did we address this and I just forgot
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r660706360
##########
File path: spark/src/test/java/org/apache/iceberg/spark/data/AvroDataTest.java
##########
@@ -40,6 +42,8 @@
protected abstract void writeAndValidate(Schema schema) throws IOException;
+ protected abstract SparkSession getSparkSession();
Review comment:
Do we need this? Just wondering if we can just do SparkSession.getActive or just change our SparkSession creation code in the testbase to always set the handle TS.withoutZone flag to true
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r664648765
##########
File path: spark3/src/main/java/org/apache/iceberg/spark/source/SparkBatchScan.java
##########
@@ -159,7 +167,7 @@ public PartitionReaderFactory createReaderFactory() {
boolean batchReadsEnabled = batchReadsEnabled(allParquetFileScanTasks, allOrcFileScanTasks);
boolean readUsingBatch = batchReadsEnabled && hasNoDeleteFiles && (allOrcFileScanTasks ||
- (allParquetFileScanTasks && atLeastOneColumn && onlyPrimitives));
+ (allParquetFileScanTasks && atLeastOneColumn && onlyPrimitives));
Review comment:
whitespace change
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r670207223
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkFixupTimestampType.java
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.iceberg.spark;
+
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.types.FixupTypes;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
+
+/**
+ * By default Spark type {@link org.apache.iceberg.types.Types.TimestampType} should be converted to
+ * {@link Types.TimestampType#withZone()} iceberg type. But we also can convert
+ * {@link org.apache.iceberg.types.Types.TimestampType} to {@link Types.TimestampType#withoutZone()} iceberg type
+ * by setting {@link SparkUtil#USE_TIMESTAMP_WITHOUT_TIME_ZONE_IN_NEW_TABLES} to 'true'
+ */
+class SparkFixupTimestampType extends FixupTypes {
+
+ private SparkFixupTimestampType(Schema referenceSchema) {
+ super(referenceSchema);
+ }
+
+ static Schema fixup(Schema schema) {
+ return new Schema(TypeUtil.visit(schema,
+ new SparkFixupTimestampType(schema)).asStructType().fields());
Review comment:
Thanks a lot, I am using IntelliJ so it is really helpful
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r660705042
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkFixupTimestampType.java
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.iceberg.spark;
+
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.types.FixupTypes;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
+
+/**
+ * By default Spark TimestampType should be converted to Types.TimestampType.withZone() iceberg type.
+ * But we also can convert TimestampType to Types.TimestampType.withoutZone() iceberg type
+ * by setting SparkUtil.STORE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY to 'true'
+ */
+class SparkFixupTimestampType extends FixupTypes {
+
+ private final boolean useTimestampWithoutZone;
+
+ private SparkFixupTimestampType(Schema referenceSchema, boolean useTimestampWithoutZone) {
Review comment:
Make sense, I will change the code
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r670191691
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkFixupTimestampType.java
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.iceberg.spark;
+
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.types.FixupTypes;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
+
+/**
+ * By default Spark type {@link org.apache.iceberg.types.Types.TimestampType} should be converted to
+ * {@link Types.TimestampType#withZone()} iceberg type. But we also can convert
+ * {@link org.apache.iceberg.types.Types.TimestampType} to {@link Types.TimestampType#withoutZone()} iceberg type
+ * by setting {@link SparkUtil#USE_TIMESTAMP_WITHOUT_TIME_ZONE_IN_NEW_TABLES} to 'true'
+ */
+class SparkFixupTimestampType extends FixupTypes {
+
+ private SparkFixupTimestampType(Schema referenceSchema) {
+ super(referenceSchema);
+ }
+
+ static Schema fixup(Schema schema) {
+ return new Schema(TypeUtil.visit(schema,
+ new SparkFixupTimestampType(schema)).asStructType().fields());
Review comment:
Nit: I think this is overly indented.
When continuing an expression on a second line, we usually indent 4 spaces (as opposed to the normal 2 spaces for changes in scope etc).
So I think that `new SparkFixup…` on this line should align the n of new with the second r from return above it (4 spaces in from the start of the word `return`).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r670202652
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkFixupTimestampType.java
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.iceberg.spark;
+
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.types.FixupTypes;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
+
+/**
+ * By default Spark type {@link org.apache.iceberg.types.Types.TimestampType} should be converted to
+ * {@link Types.TimestampType#withZone()} iceberg type. But we also can convert
+ * {@link org.apache.iceberg.types.Types.TimestampType} to {@link Types.TimestampType#withoutZone()} iceberg type
+ * by setting {@link SparkUtil#USE_TIMESTAMP_WITHOUT_TIME_ZONE_IN_NEW_TABLES} to 'true'
+ */
+class SparkFixupTimestampType extends FixupTypes {
+
+ private SparkFixupTimestampType(Schema referenceSchema) {
+ super(referenceSchema);
+ }
+
+ static Schema fixup(Schema schema) {
+ return new Schema(TypeUtil.visit(schema,
+ new SparkFixupTimestampType(schema)).asStructType().fields());
Review comment:
If you use IntelliJ, you can have it set up to point to the style guide and auto format it for you: http://iceberg.apache.org/community/#setting-up-ide-and-code-style
Otherwise, in general I believe the rules come from here: https://github.com/apache/iceberg/blob/master/.baseline/idea/intellij-java-palantir-style.xml
There are admittedly a number of these cases, so auto formatting seems like the best idea. If you don’t use IntelliJ, I think this command can be run from command line somehow but I’m not 100% sure.
Thanks for all the work on this so far. It’s very close!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r664653012
##########
File path: spark3/src/test/java/org/apache/iceberg/spark/sql/TestTimestampWithoutZone.java
##########
@@ -0,0 +1,193 @@
+/*
+ * 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.iceberg.spark.sql;
+
+import java.sql.Timestamp;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+import org.apache.iceberg.AssertHelpers;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.relocated.com.google.common.base.Joiner;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.spark.SparkCatalogTestBase;
+import org.apache.iceberg.spark.SparkSessionCatalog;
+import org.apache.iceberg.spark.SparkUtil;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.Types;
+import org.apache.spark.sql.util.CaseInsensitiveStringMap;
+import org.joda.time.DateTime;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runners.Parameterized;
+
+public class TestTimestampWithoutZone extends SparkCatalogTestBase {
+
+ private static final String newTableName = "created_table";
+ private final Map<String, String> config;
+
+ private static final Schema schema = new Schema(
+ Types.NestedField.required(1, "id", Types.LongType.get()),
+ Types.NestedField.required(2, "ts", Types.TimestampType.withoutZone()),
+ Types.NestedField.required(3, "tsz", Types.TimestampType.withZone())
+ );
+
+ private final List<Object[]> values = ImmutableList.of(
+ row(1L, toTimestamp("2021-01-01T00:00:00.0"), toTimestamp("2021-02-01T00:00:00.0")),
+ row(2L, toTimestamp("2021-01-01T00:00:00.0"), toTimestamp("2021-02-01T00:00:00.0")),
+ row(3L, toTimestamp("2021-01-01T00:00:00.0"), toTimestamp("2021-02-01T00:00:00.0"))
+ );
+
+ @Parameterized.Parameters(name = "catalogName = {0}, implementation = {1}, config = {2}")
+ public static Object[][] parameters() {
+ return new Object[][]{{"spark_catalog",
+ SparkSessionCatalog.class.getName(),
+ ImmutableMap.of(
+ "type", "hive",
+ "default-namespace", "default",
+ "parquet-enabled", "true",
+ "cache-enabled", "false"
+ )}
+ };
+ }
+
+ public TestTimestampWithoutZone(String catalogName, String implementation, Map<String, String> config) {
+ super(catalogName, implementation, config);
+ this.config = config;
+ }
+
+ @Before
+ public void createTables() {
+ spark.conf().set(SparkUtil.HANDLE_TIMESTAMP_WITHOUT_TIMEZONE, "false");
+ validationCatalog.createTable(tableIdent, schema);
+ }
+
+ @After
+ public void removeTables() {
+ validationCatalog.dropTable(tableIdent, true);
+ sql("DROP TABLE IF EXISTS %s", newTableName);
+ }
+
+ @Test
+ public void testWriteTimestampWithoutZoneError() {
+ String errorMessage = String.format("Write operation performed on a timestamp without timezone field while " +
+ "'%s' set to false should throw exception", SparkUtil.HANDLE_TIMESTAMP_WITHOUT_TIMEZONE);
+ Runnable insert = () -> sql("INSERT INTO %s VALUES %s", tableName, rowToSqlValues(values));
Review comment:
You can just put this right into the AssertThrows, we usually do this.
```
AssertThrows(errorMessage, IllegalArguementException.class, SparkUtil.TIMESTAMP_WITHOUT_TIMEZONE_ERROR,
() -> code);
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r661532432
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkTypeToType.java
##########
@@ -147,7 +147,6 @@ public Type atomic(DataType atomic) {
} else if (atomic instanceof TimestampType) {
return Types.TimestampType.withZone();
-
Review comment:
nit: noop change
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r664708831
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkUtil.java
##########
@@ -20,21 +20,38 @@
package org.apache.iceberg.spark;
import java.util.List;
+import java.util.Map;
import java.util.function.BiFunction;
import java.util.function.Function;
import java.util.stream.Collectors;
import org.apache.iceberg.PartitionField;
import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
import org.apache.iceberg.Table;
import org.apache.iceberg.hadoop.HadoopFileIO;
import org.apache.iceberg.io.FileIO;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.transforms.Transform;
import org.apache.iceberg.transforms.UnknownTransform;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.Pair;
+import org.apache.spark.sql.RuntimeConfig;
import org.apache.spark.util.SerializableConfiguration;
public class SparkUtil {
+
+ public static final String HANDLE_TIMESTAMP_WITHOUT_TIMEZONE =
+ "spark.sql.iceberg.convert-timestamp-without-timezone";
Review comment:
convert -> handle? Just to match the variable?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r660676036
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkUtil.java
##########
@@ -20,21 +20,38 @@
package org.apache.iceberg.spark;
import java.util.List;
+import java.util.Map;
import java.util.function.BiFunction;
import java.util.function.Function;
import java.util.stream.Collectors;
import org.apache.iceberg.PartitionField;
import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
import org.apache.iceberg.Table;
import org.apache.iceberg.hadoop.HadoopFileIO;
import org.apache.iceberg.io.FileIO;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.transforms.Transform;
import org.apache.iceberg.transforms.UnknownTransform;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.Pair;
+import org.apache.spark.sql.RuntimeConfig;
import org.apache.spark.util.SerializableConfiguration;
public class SparkUtil {
+
+ public static final String HANDLE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY =
+ "spark.sql.iceberg.convert-timestamp-without-timezone";
+ public static final String TIMESTAMP_WITHOUT_TIMEZONE_ERROR = String.format("Cannot handle timestamp without" +
+ " timezone fields in Spark. Spark does not natively support this type but if you would like to handle all" +
+ " timestamps as timestamp with timezone set '%s' to true. This will not change the underlying values stored" +
+ " but will change their displayed values in Spark. For more information please see" +
+ " https://docs.databricks.com/spark/latest/dataframes-datasets/dates-timestamps.html#ansi-sql-and" +
+ "-spark-sql-timestamps", HANDLE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY);
+ public static final String READ_TIMESTAMP_AS_TIMESTAMP_WITHOUT_TIMEZONE =
Review comment:
I still think we only really need one property, I think we should just use "handle" for both reading and writing
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r661504527
##########
File path: spark/src/test/java/org/apache/iceberg/spark/data/AvroDataTest.java
##########
@@ -40,6 +42,8 @@
protected abstract void writeAndValidate(Schema schema) throws IOException;
+ protected abstract SparkSession getSparkSession();
Review comment:
Agree, I have removed `getSparkSession()` abstract method, and just used `SparkSession.getActive` in test
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r662034839
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkUtil.java
##########
@@ -100,4 +117,55 @@ public static void validatePartitionTransforms(PartitionSpec spec) {
}
}
}
+
+ /**
+ * Responsible for checking if the table schema has a timestamp without timezone column
+ * @param schema table schema to check if it contains a timestamp without timezone column
+ * @return boolean indicating if the schema passed in has a timestamp field without a timezone
+ */
+ public static boolean hasTimestampWithoutZone(Schema schema) {
+ return TypeUtil.find(schema, t -> Types.TimestampType.withoutZone().equals(t)) != null;
+ }
+
+ /**
+ * Allow reading timestamp without time zone as timestamp with time zone. Generally, this is not safe as timestamp
+ * without time zone is supposed to represent wall clock time semantics, i.e. no matter the reader/writer timezone
+ * 3PM should always be read as 3PM, but timestamp with time zone represents instant semantics, i.e the timestamp
+ * is adjusted so that the corresponding time in the reader timezone is displayed.
+ * When set to false (default), we throw an exception at runtime
+ * "Spark does not support timestamp without time zone fields" if reading timestamp without time zone fields
+ *
+ * @param readerConfig table read options
+ * @param sessionConf spark session configurations
+ * @return boolean indicating if reading timestamps without timezone is allowed
+ */
+ public static boolean canHandleTimestampWithoutZone(Map<String, String> readerConfig, RuntimeConfig sessionConf) {
+ String readerOption = readerConfig.get(HANDLE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY);
+ if (readerOption != null) {
+ return Boolean.parseBoolean(readerOption);
+ }
+ String sessionConfValue = sessionConf.get(HANDLE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY, null);
+ if (sessionConfValue != null) {
+ return Boolean.parseBoolean(sessionConfValue);
+ }
+ return false;
+ }
+
+ /**
+ * Check whether the spark session config contains a 'spark.sql.iceberg.read-timestamp-as-timestamp-without-timezone'
+ * property.
+ * Default value - false
+ *
+ * @param sessionConf a spark runtime config
+ * @return true if the session config has spark.sql.iceberg.read-timestamp-as-timestamp-without-timezone property
+ * and this property is set to true
+ */
+ public static boolean shouldStoreTimestampWithoutZone(RuntimeConfig sessionConf) {
+ String sessionConfValue = sessionConf.get(READ_TIMESTAMP_AS_TIMESTAMP_WITHOUT_TIMEZONE, null);
Review comment:
for this case this is right property. This method checks should we use `Types.TimestampType#withoutZone()` or `Types.TimestampType#withZone()` in new tables.
Changed method name and added little bit more details in doc
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r660682221
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkFixupTimestampType.java
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.iceberg.spark;
+
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.types.FixupTypes;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
+
+/**
+ * By default Spark TimestampType should be converted to Types.TimestampType.withZone() iceberg type.
+ * But we also can convert TimestampType to Types.TimestampType.withoutZone() iceberg type
+ * by setting SparkUtil.STORE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY to 'true'
+ */
+class SparkFixupTimestampType extends FixupTypes {
+
+ private final boolean useTimestampWithoutZone;
+
+ private SparkFixupTimestampType(Schema referenceSchema, boolean useTimestampWithoutZone) {
Review comment:
Do we need this class to be parameterized? Can't we just not use it if the property isn't set?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r670198977
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkFixupTimestampType.java
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.iceberg.spark;
+
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.types.FixupTypes;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
+
+/**
+ * By default Spark type {@link org.apache.iceberg.types.Types.TimestampType} should be converted to
+ * {@link Types.TimestampType#withZone()} iceberg type. But we also can convert
+ * {@link org.apache.iceberg.types.Types.TimestampType} to {@link Types.TimestampType#withoutZone()} iceberg type
+ * by setting {@link SparkUtil#USE_TIMESTAMP_WITHOUT_TIME_ZONE_IN_NEW_TABLES} to 'true'
+ */
+class SparkFixupTimestampType extends FixupTypes {
+
+ private SparkFixupTimestampType(Schema referenceSchema) {
+ super(referenceSchema);
+ }
+
+ static Schema fixup(Schema schema) {
+ return new Schema(TypeUtil.visit(schema,
+ new SparkFixupTimestampType(schema)).asStructType().fields());
Review comment:
I have pushed fix
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#issuecomment-879055604
> @sshkvar for future reference you can use `g build -x test -x integrationTest -x testSpark31` to run the checkstyle and other static analysis tests
ok, thanks
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r662033480
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkTypeToType.java
##########
@@ -147,7 +147,6 @@ public Type atomic(DataType atomic) {
} else if (atomic instanceof TimestampType) {
return Types.TimestampType.withZone();
-
Review comment:
oops, sorry, fixed
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r661505008
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkFixupTimestampType.java
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.iceberg.spark;
+
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.types.FixupTypes;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
+
+/**
+ * By default Spark TimestampType should be converted to Types.TimestampType.withZone() iceberg type.
+ * But we also can convert TimestampType to Types.TimestampType.withoutZone() iceberg type
+ * by setting SparkUtil.STORE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY to 'true'
Review comment:
good point, fixed
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r661502786
##########
File path: spark2/src/main/java/org/apache/iceberg/spark/source/Reader.java
##########
@@ -329,8 +335,10 @@ public boolean enableBatchRead() {
boolean batchReadsEnabled = batchReadsEnabled(allParquetFileScanTasks, allOrcFileScanTasks);
+ boolean hasTimestampWithoutZone = SparkUtil.hasTimestampWithoutZone(lazySchema());
+
this.readUsingBatch = batchReadsEnabled && hasNoDeleteFiles && (allOrcFileScanTasks ||
- (allParquetFileScanTasks && atLeastOneColumn && onlyPrimitives));
+ (allParquetFileScanTasks && atLeastOneColumn && onlyPrimitives && !hasTimestampWithoutZone));
Review comment:
This code was added by @bkahloon, but I agree that we don't need this check.
I have removed it
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r670197715
##########
File path: spark2/src/main/java/org/apache/iceberg/spark/source/Reader.java
##########
@@ -183,6 +187,8 @@ private Expression filterExpression() {
private StructType lazyType() {
if (type == null) {
+ Preconditions.checkArgument(readTimestampWithoutZone || !SparkUtil.hasTimestampWithoutZone(lazySchema()),
+ SparkUtil.TIMESTAMP_WITHOUT_TIMEZONE_ERROR);
Review comment:
Would it make sense to add this check into `SparkSchemaUtil.convert`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#issuecomment-880099853
Check failed
```
org.apache.iceberg.actions.TestDeleteReachableFilesAction24 > dataFilesCleanupWithParallelTasks FAILED
java.lang.AssertionError: FILE_A should be deleted
```
Locally this test passed successfully. @RussellSpitzer Can we try to rerun this step?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r661517291
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkFixupTimestampType.java
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.iceberg.spark;
+
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.types.FixupTypes;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
+
+/**
+ * By default Spark TimestampType should be converted to Types.TimestampType.withZone() iceberg type.
+ * But we also can convert TimestampType to Types.TimestampType.withoutZone() iceberg type
+ * by setting SparkUtil.STORE_TIMESTAMP_WITHOUT_TIMEZONE_SESSION_PROPERTY to 'true'
+ */
+class SparkFixupTimestampType extends FixupTypes {
+
+ private final boolean useTimestampWithoutZone;
+
+ private SparkFixupTimestampType(Schema referenceSchema, boolean useTimestampWithoutZone) {
Review comment:
Let me provide some details why we need to have ability to select which timestamp type (`Types.TimestampType.withZone()` or `Types.TimestampType.withoutZone()`) will be used for **new** iceberg tables created from spark.
We have 2 main engines: Trino and Spark. In Trino we uses only timestamp types (without timezone), so we need to have ability to create new iceberg tables from spark with TS without time zone, just to have ability to not change a lot of our existing ETL processes
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r670214102
##########
File path: spark/src/test/java/org/apache/iceberg/spark/data/AvroDataTest.java
##########
@@ -185,4 +190,51 @@ public void testMixedTypes() throws IOException {
writeAndValidate(schema);
}
+
+ @Test
+ public void testTimestampWithoutZone() throws IOException {
+ withSQLConf(ImmutableMap.of(SparkUtil.HANDLE_TIMESTAMP_WITHOUT_TIMEZONE, "true"), () -> {
+ Schema schema = TypeUtil.assignIncreasingFreshIds(new Schema(
+ required(0, "id", LongType.get()),
+ optional(1, "ts_without_zone", Types.TimestampType.withoutZone())));
+
+ writeAndValidate(schema);
+ });
+ }
+
+ protected void withSQLConf(Map<String, String> conf, Action action) throws IOException {
+ SQLConf sqlConf = SQLConf.get();
+
+ Map<String, String> currentConfValues = Maps.newHashMap();
+ conf.keySet().forEach(confKey -> {
+ if (sqlConf.contains(confKey)) {
+ String currentConfValue = sqlConf.getConfString(confKey);
+ currentConfValues.put(confKey, currentConfValue);
+ }
+ });
+
+ conf.forEach((confKey, confValue) -> {
+ if (SQLConf.staticConfKeys().contains(confKey)) {
+ throw new RuntimeException("Cannot modify the value of a static config: " + confKey);
+ }
+ sqlConf.setConfString(confKey, confValue);
+ });
+
+ try {
+ action.invoke();
+ } finally {
+ conf.forEach((confKey, confValue) -> {
+ if (currentConfValues.containsKey(confKey)) {
+ sqlConf.setConfString(confKey, currentConfValues.get(confKey));
+ } else {
+ sqlConf.unsetConf(confKey);
+ }
+ });
+ }
+ }
+
+ @FunctionalInterface
+ protected interface Action {
+ void invoke() throws IOException;
+ }
Review comment:
As a follow up PR (in a separate PR either before or after this one is merged), particularly if you're looking for some more work to do to contribute to the project, you _might_ explore if this combination of `withSQLConf` and the corresponding `@FunctionalInterface` can be abstracted into their own interface that one could mix into tests.
I'm not 100% sure how that would look, maybe an interface like `ConfigurableTestSQLConf` or something?
Again, just copying it for now is fine, but it would be nice to reduce the code duplication and make this easier for others to use in the future. Your exploration might find that it’s better to not do that (I’m more of a Scala developer myself and so to me it feels like a mixin). Something to think about for later though!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r660712924
##########
File path: spark2/src/main/java/org/apache/iceberg/spark/source/Reader.java
##########
@@ -329,8 +335,10 @@ public boolean enableBatchRead() {
boolean batchReadsEnabled = batchReadsEnabled(allParquetFileScanTasks, allOrcFileScanTasks);
+ boolean hasTimestampWithoutZone = SparkUtil.hasTimestampWithoutZone(lazySchema());
+
this.readUsingBatch = batchReadsEnabled && hasNoDeleteFiles && (allOrcFileScanTasks ||
- (allParquetFileScanTasks && atLeastOneColumn && onlyPrimitives));
+ (allParquetFileScanTasks && atLeastOneColumn && onlyPrimitives && !hasTimestampWithoutZone));
Review comment:
wasI think I noted on the other PR I don't think we need this
##########
File path: spark2/src/main/java/org/apache/iceberg/spark/source/Reader.java
##########
@@ -329,8 +335,10 @@ public boolean enableBatchRead() {
boolean batchReadsEnabled = batchReadsEnabled(allParquetFileScanTasks, allOrcFileScanTasks);
+ boolean hasTimestampWithoutZone = SparkUtil.hasTimestampWithoutZone(lazySchema());
+
this.readUsingBatch = batchReadsEnabled && hasNoDeleteFiles && (allOrcFileScanTasks ||
- (allParquetFileScanTasks && atLeastOneColumn && onlyPrimitives));
+ (allParquetFileScanTasks && atLeastOneColumn && onlyPrimitives && !hasTimestampWithoutZone));
Review comment:
I think I noted on the other PR I don't think we need this
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] sshkvar commented on a change in pull request #2757: Add support for reading/writing timestamps without timezone.
Posted by GitBox <gi...@apache.org>.
sshkvar commented on a change in pull request #2757:
URL: https://github.com/apache/iceberg/pull/2757#discussion_r670194841
##########
File path: spark/src/main/java/org/apache/iceberg/spark/SparkFixupTimestampType.java
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.iceberg.spark;
+
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.types.FixupTypes;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
+
+/**
+ * By default Spark type {@link org.apache.iceberg.types.Types.TimestampType} should be converted to
+ * {@link Types.TimestampType#withZone()} iceberg type. But we also can convert
+ * {@link org.apache.iceberg.types.Types.TimestampType} to {@link Types.TimestampType#withoutZone()} iceberg type
+ * by setting {@link SparkUtil#USE_TIMESTAMP_WITHOUT_TIME_ZONE_IN_NEW_TABLES} to 'true'
+ */
+class SparkFixupTimestampType extends FixupTypes {
+
+ private SparkFixupTimestampType(Schema referenceSchema) {
+ super(referenceSchema);
+ }
+
+ static Schema fixup(Schema schema) {
+ return new Schema(TypeUtil.visit(schema,
+ new SparkFixupTimestampType(schema)).asStructType().fields());
Review comment:
I will fix it asap, where can I find code style guide?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org