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/12/29 09:46:01 UTC
[GitHub] [iceberg] hililiwei opened a new pull request #3819: Add a unit test to write-read-validate via FlinkParquetWrite/Reader
hililiwei opened a new pull request #3819:
URL: https://github.com/apache/iceberg/pull/3819
TestFlinkParquetWriter uses FlinkParquetWriters to write data, uses GenericParquetReaders to read data.
TestFlinkParquetReader uses FlinkParquetReaders to read data, uses GenericParquetWriter to write data.
Added a test named TestFlinkParquetReaderWriter, FlinkParquetWriters is used to write and FlinkParquetReaders to read.
--
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] hililiwei commented on a change in pull request #3819: Flink: Add a unit test to write-read-validate via FlinkParquetWrite/Reader
Posted by GitBox <gi...@apache.org>.
hililiwei commented on a change in pull request #3819:
URL: https://github.com/apache/iceberg/pull/3819#discussion_r776773223
##########
File path: flink/v1.14/flink/src/main/java/org/apache/iceberg/flink/data/FlinkParquetReaders.java
##########
@@ -284,7 +284,6 @@ private FlinkParquetReaders() {
public DecimalData read(DecimalData ignored) {
Binary binary = column.nextBinary();
BigDecimal bigDecimal = new BigDecimal(new BigInteger(binary.getBytes()), scale);
- // TODO: need a unit test to write-read-validate decimal via FlinkParquetWrite/Reader
Review comment:
The org.apache.iceberg.data.DataTest#testSimpleStruct already contains data of decimal type, so I did not add a separate test case for that.
https://github.com/apache/iceberg/blob/1474f8408aefc031ec271399859bc51cb2e96f8e/data/src/test/java/org/apache/iceberg/data/DataTest.java#L42-L68
--
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] rdblue commented on a change in pull request #3819: Flink: Add a unit test to write-read-validate via FlinkParquetWrite/Reader
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3819:
URL: https://github.com/apache/iceberg/pull/3819#discussion_r776511154
##########
File path: flink/v1.14/flink/src/test/java/org/apache/iceberg/flink/data/TestFlinkParquetReaderWriter.java
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.flink.data;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.Iterator;
+import org.apache.flink.table.data.RowData;
+import org.apache.flink.table.types.logical.LogicalType;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.data.DataTest;
+import org.apache.iceberg.data.RandomGenericData;
+import org.apache.iceberg.flink.FlinkSchemaUtil;
+import org.apache.iceberg.flink.TestHelpers;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.parquet.Parquet;
+import org.junit.Assert;
+
+public class TestFlinkParquetReaderWriter extends DataTest {
Review comment:
What is this doing that isn't already accomplished by `TestFlinkParquetReader` or `TestFlinkParquetWriter`?
--
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] rdblue commented on pull request #3819: Flink: Add a unit test to write-read-validate via FlinkParquetWrite/Reader
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3819:
URL: https://github.com/apache/iceberg/pull/3819#issuecomment-1002793960
@hililiwei, there is no need to open an issue when you intend to open a PR soon after. There's no requirement that a PR is associated with an issue and creating an issue with the same summary and description as a PR is just creates unnecessary work. Feel free to just open the PR directly next time.
--
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] hililiwei commented on a change in pull request #3819: Flink: Add a unit test to write-read-validate via FlinkParquetWrite/Reader
Posted by GitBox <gi...@apache.org>.
hililiwei commented on a change in pull request #3819:
URL: https://github.com/apache/iceberg/pull/3819#discussion_r776777164
##########
File path: flink/v1.14/flink/src/test/java/org/apache/iceberg/flink/data/TestFlinkParquetReaderWriter.java
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.flink.data;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.Iterator;
+import org.apache.flink.table.data.RowData;
+import org.apache.flink.table.types.logical.LogicalType;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.data.DataTest;
+import org.apache.iceberg.data.RandomGenericData;
+import org.apache.iceberg.flink.FlinkSchemaUtil;
+import org.apache.iceberg.flink.TestHelpers;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.parquet.Parquet;
+import org.junit.Assert;
+
+public class TestFlinkParquetReaderWriter extends DataTest {
Review comment:
As mentioned above, when I read this part of code, I found that:
- The TODO describes that a unit test needs to be added to verify decimal type writer\read, but org.apache.iceberg.data.DataTest#testSimpleStruct seems contains this type. Is this enough?
- There is no unit test that uses both FlinkParquetWrite and FlinkParquetWriteReader.
Based on this, I submitted this PR. If this is not necessary or my understanding is incorrect, let me close 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] rdblue commented on pull request #3819: Flink: Add a unit test to write-read-validate via FlinkParquetWrite/Reader
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3819:
URL: https://github.com/apache/iceberg/pull/3819#issuecomment-1003200641
I'm going to close this. I think that we already have good coverage of the readers and writers by writing generics and reading Flink rows, or writing Flink rows and reading generics. Those are better tests because they isolate what is being tested.
--
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] rdblue commented on a change in pull request #3819: Flink: Add a unit test to write-read-validate via FlinkParquetWrite/Reader
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3819:
URL: https://github.com/apache/iceberg/pull/3819#discussion_r776508904
##########
File path: flink/v1.14/flink/src/main/java/org/apache/iceberg/flink/data/FlinkParquetReaders.java
##########
@@ -284,7 +284,6 @@ private FlinkParquetReaders() {
public DecimalData read(DecimalData ignored) {
Binary binary = column.nextBinary();
BigDecimal bigDecimal = new BigDecimal(new BigInteger(binary.getBytes()), scale);
- // TODO: need a unit test to write-read-validate decimal via FlinkParquetWrite/Reader
Review comment:
If this is what is being addressed by this PR, can you add decimal to the description and summary?
--
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] hililiwei commented on a change in pull request #3819: Flink: Add a unit test to write-read-validate via FlinkParquetWrite/Reader
Posted by GitBox <gi...@apache.org>.
hililiwei commented on a change in pull request #3819:
URL: https://github.com/apache/iceberg/pull/3819#discussion_r776773937
##########
File path: flink/v1.14/flink/src/main/java/org/apache/iceberg/flink/data/FlinkParquetReaders.java
##########
@@ -284,7 +284,6 @@ private FlinkParquetReaders() {
public DecimalData read(DecimalData ignored) {
Binary binary = column.nextBinary();
BigDecimal bigDecimal = new BigDecimal(new BigInteger(binary.getBytes()), scale);
- // TODO: need a unit test to write-read-validate decimal via FlinkParquetWrite/Reader
Review comment:
This PR is more about using FlinkParquetWrite and Reader to write and read data at the same time, rather than using either of them alone.
--
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] rdblue commented on a change in pull request #3819: Flink: Add a unit test to write-read-validate via FlinkParquetWrite/Reader
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3819:
URL: https://github.com/apache/iceberg/pull/3819#discussion_r776881757
##########
File path: flink/v1.14/flink/src/test/java/org/apache/iceberg/flink/data/TestFlinkParquetReaderWriter.java
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.flink.data;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.Iterator;
+import org.apache.flink.table.data.RowData;
+import org.apache.flink.table.types.logical.LogicalType;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.data.DataTest;
+import org.apache.iceberg.data.RandomGenericData;
+import org.apache.iceberg.flink.FlinkSchemaUtil;
+import org.apache.iceberg.flink.TestHelpers;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.parquet.Parquet;
+import org.junit.Assert;
+
+public class TestFlinkParquetReaderWriter extends DataTest {
Review comment:
Thanks for working on this, but I don't think that it is necessary. I think that we have plenty of coverage and better testing using the other two.
--
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] rdblue closed pull request #3819: Flink: Add a unit test to write-read-validate via FlinkParquetWrite/Reader
Posted by GitBox <gi...@apache.org>.
rdblue closed pull request #3819:
URL: https://github.com/apache/iceberg/pull/3819
--
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