You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "jbonofre (via GitHub)" <gi...@apache.org> on 2024/10/30 13:48:38 UTC
[PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
jbonofre opened a new pull request, #11430:
URL: https://github.com/apache/iceberg/pull/11430
Following #11386, this PR adds test coverage on both the "new" `FlinkAvroPlannedReader` and the "deprecated" `FlinkAvroReader`.
--
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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#discussion_r1858208865
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/data/AbstractTestFlinkAvroReaderWriter.java:
##########
@@ -64,12 +64,13 @@ public class TestFlinkAvroReaderWriter extends DataTest {
Types.NestedField.optional(8, "bigint", Types.LongType.get()),
Types.NestedField.optional(9, "decimal", Types.DecimalType.of(4, 2)));
- @Override
Review Comment:
unrelated 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
Re: [PR] Flink: Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#issuecomment-2501043725
@pvary I added `@Deprecated` to the 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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#discussion_r1853688372
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/data/TestRowProjection.java:
##########
@@ -61,52 +63,63 @@ private RowData writeAndRead(String desc, Schema writeSchema, Schema readSchema,
appender.add(row);
}
- Iterable<RowData> records =
+ Avro.ReadBuilder builder =
Avro.read(Files.localInput(file))
.project(readSchema)
- .createResolvingReader(FlinkPlannedAvroReader::create)
- .build();
+ .createReaderFunc(FlinkAvroReader::new);
+ if (useAvroPlannedReader) {
+ builder =
+ Avro.read(Files.localInput(file))
+ .project(readSchema)
+ .createResolvingReader(FlinkPlannedAvroReader::create);
+ }
+ Iterable<RowData> records = builder.build();
return Iterables.getOnlyElement(records);
}
- @Test
- public void testFullProjection() throws Exception {
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
Review Comment:
Actually, after reviewing this part, I'm not convinced `@TestTemplate` is much better: we would need to add `@TestTemplate` and `@ExtendWith()` annotations. So, still two annotations to add + the context provider.
@pvary thoughts ?
--
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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#issuecomment-2493928687
@nastra if you have time to take a look, 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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#discussion_r1827556254
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/data/AbstractTestFlinkAvroReaderWriter.java:
##########
@@ -80,20 +79,27 @@ private void writeAndValidate(Schema schema, List<Record> expectedRecords, int n
// Write the expected records into AVRO file, then read them into RowData and assert with the
// expected Record list.
- try (FileAppender<Record> writer =
+ try (FileAppender<org.apache.iceberg.data.Record> writer =
Avro.write(Files.localOutput(recordsFile))
.schema(schema)
.createWriterFunc(DataWriter::create)
.build()) {
writer.addAll(expectedRecords);
}
- try (CloseableIterable<RowData> reader =
+ Avro.ReadBuilder builder =
Avro.read(Files.localInput(recordsFile))
.project(schema)
- .createResolvingReader(FlinkPlannedAvroReader::create)
- .build()) {
- Iterator<Record> expected = expectedRecords.iterator();
+ .createReaderFunc(FlinkAvroReader::new);
+ if (useAvroPlannedReader) {
+ builder =
+ Avro.read(Files.localInput(recordsFile))
+ .project(schema)
+ .createResolvingReader(FlinkPlannedAvroReader::create);
+ }
Review Comment:
Maybe put this in an abstract method and use it inside the `writeAndValidate`?
Something like:
```
abstract Avro.ReadBuilder builder() {
```
--
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
Re: [PR] Flink: Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#issuecomment-2500885295
@RussellSpitzer do you mind to merge this one ? 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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#discussion_r1830478089
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/data/AbstractTestFlinkAvroReaderWriter.java:
##########
@@ -65,12 +63,13 @@ public class TestFlinkAvroReaderWriter extends DataTest {
Types.NestedField.optional(9, "decimal", Types.DecimalType.of(4, 2)));
@Override
- protected void writeAndValidate(Schema schema) throws IOException {
- List<Record> expectedRecords = RandomGenericData.generate(schema, NUM_RECORDS, 1991L);
- writeAndValidate(schema, expectedRecords, NUM_RECORDS);
- }
+ protected abstract void writeAndValidate(Schema schema) throws IOException;
- private void writeAndValidate(Schema schema, List<Record> expectedRecords, int numRecord)
+ void writeAndValidate(
+ Schema schema,
+ List<org.apache.iceberg.data.Record> expectedRecords,
Review Comment:
Afair, the reason is to avoid confusion with another `Record`. Let me 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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#discussion_r1830478706
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/data/TestRowProjection.java:
##########
@@ -61,52 +63,63 @@ private RowData writeAndRead(String desc, Schema writeSchema, Schema readSchema,
appender.add(row);
}
- Iterable<RowData> records =
+ Avro.ReadBuilder builder =
Avro.read(Files.localInput(file))
.project(readSchema)
- .createResolvingReader(FlinkPlannedAvroReader::create)
- .build();
+ .createReaderFunc(FlinkAvroReader::new);
+ if (useAvroPlannedReader) {
+ builder =
+ Avro.read(Files.localInput(file))
+ .project(readSchema)
+ .createResolvingReader(FlinkPlannedAvroReader::create);
+ }
+ Iterable<RowData> records = builder.build();
return Iterables.getOnlyElement(records);
}
- @Test
- public void testFullProjection() throws Exception {
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
Review Comment:
Let me check, but I think `@TestTemplate` is an option.
--
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
Re: [PR] Flink: Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#issuecomment-2501235249
Thanks @jbonofre for the PR and @nastra for the review.
@jbonofre: Please port the changes to Flink 1.19/1.18 too.
Thanks,
Peter
--
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
Re: [PR] Flink: Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#issuecomment-2500917415
@pvary I may have missed your comment (about deprecated message) 😀
--
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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#discussion_r1858040245
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/data/TestRowProjection.java:
##########
@@ -61,52 +63,63 @@ private RowData writeAndRead(String desc, Schema writeSchema, Schema readSchema,
appender.add(row);
}
- Iterable<RowData> records =
+ Avro.ReadBuilder builder =
Avro.read(Files.localInput(file))
.project(readSchema)
- .createResolvingReader(FlinkPlannedAvroReader::create)
- .build();
+ .createReaderFunc(FlinkAvroReader::new);
+ if (useAvroPlannedReader) {
+ builder =
+ Avro.read(Files.localInput(file))
+ .project(readSchema)
+ .createResolvingReader(FlinkPlannedAvroReader::create);
+ }
+ Iterable<RowData> records = builder.build();
return Iterables.getOnlyElement(records);
}
- @Test
- public void testFullProjection() throws Exception {
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
Review Comment:
I update the test to use `@TestTemplate`.
--
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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#discussion_r1858210557
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/data/AbstractTestFlinkAvroReaderWriter.java:
##########
@@ -88,11 +89,9 @@ private void writeAndValidate(Schema schema, List<Record> expectedRecords, int n
writer.addAll(expectedRecords);
}
- try (CloseableIterable<RowData> reader =
- Avro.read(Files.localInput(recordsFile))
- .project(schema)
- .createResolvingReader(FlinkPlannedAvroReader::create)
- .build()) {
+ Avro.ReadBuilder builder = createAvroReadBuilder(recordsFile, schema);
+
+ try (CloseableIterable<RowData> reader = builder.build()) {
Review Comment:
```suggestion
try (CloseableIterable<RowData> reader = createAvroReadBuilder(recordsFile, schema).build()) {
```
--
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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#discussion_r1855135884
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/data/AbstractTestFlinkAvroReaderWriter.java:
##########
@@ -80,20 +77,18 @@ private void writeAndValidate(Schema schema, List<Record> expectedRecords, int n
// Write the expected records into AVRO file, then read them into RowData and assert with the
// expected Record list.
- try (FileAppender<Record> writer =
+ try (FileAppender<org.apache.iceberg.data.Record> writer =
Review Comment:
No, we can use the short name. I will update.
--
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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#issuecomment-2493711948
@Fokko @pvary @RussellSpitzer I updated the tests according to Peter's comment. I'm just not convinced that using `@TestTemplate` is much better than `@ParameterizedTest` 😄
--
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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#discussion_r1830478303
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/data/AbstractTestFlinkAvroReaderWriter.java:
##########
@@ -80,20 +79,27 @@ private void writeAndValidate(Schema schema, List<Record> expectedRecords, int n
// Write the expected records into AVRO file, then read them into RowData and assert with the
// expected Record list.
- try (FileAppender<Record> writer =
+ try (FileAppender<org.apache.iceberg.data.Record> writer =
Avro.write(Files.localOutput(recordsFile))
.schema(schema)
.createWriterFunc(DataWriter::create)
.build()) {
writer.addAll(expectedRecords);
}
- try (CloseableIterable<RowData> reader =
+ Avro.ReadBuilder builder =
Avro.read(Files.localInput(recordsFile))
.project(schema)
- .createResolvingReader(FlinkPlannedAvroReader::create)
- .build()) {
- Iterator<Record> expected = expectedRecords.iterator();
+ .createReaderFunc(FlinkAvroReader::new);
+ if (useAvroPlannedReader) {
+ builder =
+ Avro.read(Files.localInput(recordsFile))
+ .project(schema)
+ .createResolvingReader(FlinkPlannedAvroReader::create);
+ }
Review Comment:
Good idea. Let me do that. 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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#discussion_r1854035536
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/data/TestRowProjection.java:
##########
@@ -61,52 +63,63 @@ private RowData writeAndRead(String desc, Schema writeSchema, Schema readSchema,
appender.add(row);
}
- Iterable<RowData> records =
+ Avro.ReadBuilder builder =
Avro.read(Files.localInput(file))
.project(readSchema)
- .createResolvingReader(FlinkPlannedAvroReader::create)
- .build();
+ .createReaderFunc(FlinkAvroReader::new);
+ if (useAvroPlannedReader) {
+ builder =
+ Avro.read(Files.localInput(file))
+ .project(readSchema)
+ .createResolvingReader(FlinkPlannedAvroReader::create);
+ }
+ Iterable<RowData> records = builder.build();
return Iterables.getOnlyElement(records);
}
- @Test
- public void testFullProjection() throws Exception {
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
Review Comment:
OK, let me do 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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#discussion_r1858164749
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/data/AbstractTestFlinkAvroReaderWriter.java:
##########
@@ -154,9 +149,7 @@ private Record recordNumType(
return record;
}
- @Test
- public void testNumericTypes() throws IOException {
-
+ void testNumericTypes() throws IOException {
Review Comment:
it seems this became a simple method that isn't being tested anymore?
--
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
Re: [PR] Flink: Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#discussion_r1858276460
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/data/TestFlinkAvroDeprecatedReaderWriter.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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 org.apache.iceberg.Files;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.avro.Avro;
+
+public class TestFlinkAvroDeprecatedReaderWriter extends AbstractTestFlinkAvroReaderWriter {
Review Comment:
Nit: could we add a deprecated annotation and a comment:
```
/**
* @deprecated should be removed in 1.8.0; along with FlinkAvroReader.
*/
@Deprecated
```
--
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
Re: [PR] Flink: Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#issuecomment-2500905782
@jbonofre: If the deprecated message is there, then I can merge.. no problem
--
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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#discussion_r1827559789
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/data/TestRowProjection.java:
##########
@@ -61,52 +63,63 @@ private RowData writeAndRead(String desc, Schema writeSchema, Schema readSchema,
appender.add(row);
}
- Iterable<RowData> records =
+ Avro.ReadBuilder builder =
Avro.read(Files.localInput(file))
.project(readSchema)
- .createResolvingReader(FlinkPlannedAvroReader::create)
- .build();
+ .createReaderFunc(FlinkAvroReader::new);
+ if (useAvroPlannedReader) {
+ builder =
+ Avro.read(Files.localInput(file))
+ .project(readSchema)
+ .createResolvingReader(FlinkPlannedAvroReader::create);
+ }
+ Iterable<RowData> records = builder.build();
return Iterables.getOnlyElement(records);
}
- @Test
- public void testFullProjection() throws Exception {
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
Review Comment:
Do we add this to all of the tests? Maybe use `@TestTemplate` for 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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#discussion_r1854033609
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/data/TestRowProjection.java:
##########
@@ -61,52 +63,63 @@ private RowData writeAndRead(String desc, Schema writeSchema, Schema readSchema,
appender.add(row);
}
- Iterable<RowData> records =
+ Avro.ReadBuilder builder =
Avro.read(Files.localInput(file))
.project(readSchema)
- .createResolvingReader(FlinkPlannedAvroReader::create)
- .build();
+ .createReaderFunc(FlinkAvroReader::new);
+ if (useAvroPlannedReader) {
+ builder =
+ Avro.read(Files.localInput(file))
+ .project(readSchema)
+ .createResolvingReader(FlinkPlannedAvroReader::create);
+ }
+ Iterable<RowData> records = builder.build();
return Iterables.getOnlyElement(records);
}
- @Test
- public void testFullProjection() throws Exception {
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
Review Comment:
`@TestTemplate` is for parameterization at the class level, which seems exactly what's being done here. So rather than adding `@ValueSource` to each single method it makes sense to do this at the class level and annotate all tests with `@TestTemplate`
--
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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#discussion_r1854047740
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/data/TestRowProjection.java:
##########
@@ -61,52 +63,63 @@ private RowData writeAndRead(String desc, Schema writeSchema, Schema readSchema,
appender.add(row);
}
- Iterable<RowData> records =
+ Avro.ReadBuilder builder =
Avro.read(Files.localInput(file))
.project(readSchema)
- .createResolvingReader(FlinkPlannedAvroReader::create)
- .build();
+ .createReaderFunc(FlinkAvroReader::new);
+ if (useAvroPlannedReader) {
+ builder =
+ Avro.read(Files.localInput(file))
+ .project(readSchema)
+ .createResolvingReader(FlinkPlannedAvroReader::create);
+ }
+ Iterable<RowData> records = builder.build();
return Iterables.getOnlyElement(records);
}
- @Test
- public void testFullProjection() throws Exception {
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
Review Comment:
@nastra by the way, I was confused about the expectation here because `@TestTemplate` is not necessary at class level, it can be at method level too. Actually both `@ParameterizedTest` and `@RepeatedTest` are based on `@TestTemplate` (they use custom invocation context provider). That's why I was confused (as `@ParameterizedTest` is actually a "specialized" `@TestTemplate`) 😄
--
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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#issuecomment-2500078176
@nastra @pvary @Fokko I updated the tests according to your comments. If you can take a new look, 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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#discussion_r1858209515
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/data/AbstractTestFlinkAvroReaderWriter.java:
##########
@@ -48,11 +48,11 @@
import org.apache.iceberg.util.DateTimeUtil;
import org.junit.jupiter.api.Test;
-public class TestFlinkAvroReaderWriter extends DataTest {
+public abstract class AbstractTestFlinkAvroReaderWriter extends DataTest {
- private static final int NUM_RECORDS = 100;
+ static final int NUM_RECORDS = 100;
Review Comment:
this and the ones below are both unrelated changes I believe
--
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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#discussion_r1855131415
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/data/AbstractTestFlinkAvroReaderWriter.java:
##########
@@ -80,20 +77,18 @@ private void writeAndValidate(Schema schema, List<Record> expectedRecords, int n
// Write the expected records into AVRO file, then read them into RowData and assert with the
// expected Record list.
- try (FileAppender<Record> writer =
+ try (FileAppender<org.apache.iceberg.data.Record> writer =
Review Comment:
Do we really need the fully qualified name here? I'm reviewing on my phone, so I might miss something, but I was not able to find the reason
--
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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#issuecomment-2447225861
@RussellSpitzer as requested by @pvary and @Fokko this PR add tests for both the "old" and "new" readers.
--
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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#issuecomment-2500169112
@nastra sorry, I forgot to push some changes, it should be better now.
--
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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#discussion_r1858208354
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/data/TestRowProjection.java:
##########
@@ -61,16 +76,22 @@ private RowData writeAndRead(String desc, Schema writeSchema, Schema readSchema,
appender.add(row);
}
- Iterable<RowData> records =
+ Avro.ReadBuilder builder =
Avro.read(Files.localInput(file))
.project(readSchema)
- .createResolvingReader(FlinkPlannedAvroReader::create)
- .build();
+ .createReaderFunc(FlinkAvroReader::new);
+ if (useAvroPlannedReader) {
+ builder =
+ Avro.read(Files.localInput(file))
+ .project(readSchema)
+ .createResolvingReader(FlinkPlannedAvroReader::create);
+ }
+ Iterable<RowData> records = builder.build();
Review Comment:
nit: newline after }
--
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
Re: [PR] Flink: Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary merged PR #11430:
URL: https://github.com/apache/iceberg/pull/11430
--
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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#discussion_r1858245210
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/data/AbstractTestFlinkAvroReaderWriter.java:
##########
@@ -156,7 +154,7 @@ private Record recordNumType(
@Test
public void testNumericTypes() throws IOException {
-
+
Review Comment:
removing that line in your earlier commit was fine imo, but this is adding a tab ;)
--
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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#discussion_r1858160427
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/data/TestFlinkAvroPlannedReaderWriter.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.List;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.avro.Avro;
+import org.apache.iceberg.data.RandomGenericData;
+import org.apache.iceberg.data.Record;
+
+public class TestFlinkAvroPlannedReaderWriter extends AbstractTestFlinkAvroReaderWriter {
+
+ @Override
+ protected void writeAndValidate(Schema schema) throws IOException {
Review Comment:
not sure why both subclasses need to override this method from the super class
--
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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#discussion_r1858161532
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/data/AbstractTestFlinkAvroReaderWriter.java:
##########
@@ -65,12 +63,11 @@ public class TestFlinkAvroReaderWriter extends DataTest {
Types.NestedField.optional(9, "decimal", Types.DecimalType.of(4, 2)));
@Override
- protected void writeAndValidate(Schema schema) throws IOException {
- List<Record> expectedRecords = RandomGenericData.generate(schema, NUM_RECORDS, 1991L);
- writeAndValidate(schema, expectedRecords, NUM_RECORDS);
- }
+ protected abstract void writeAndValidate(Schema schema) throws IOException;
Review Comment:
is there a particular reason why this is being made abstract now? It seems both subclasses override and do the same thing as this method was doing previously
--
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
Re: [PR] Test both "new" Flink Avro planned reader and "deprecated" Avro reader [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #11430:
URL: https://github.com/apache/iceberg/pull/11430#discussion_r1827550972
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/data/AbstractTestFlinkAvroReaderWriter.java:
##########
@@ -65,12 +63,13 @@ public class TestFlinkAvroReaderWriter extends DataTest {
Types.NestedField.optional(9, "decimal", Types.DecimalType.of(4, 2)));
@Override
- protected void writeAndValidate(Schema schema) throws IOException {
- List<Record> expectedRecords = RandomGenericData.generate(schema, NUM_RECORDS, 1991L);
- writeAndValidate(schema, expectedRecords, NUM_RECORDS);
- }
+ protected abstract void writeAndValidate(Schema schema) throws IOException;
- private void writeAndValidate(Schema schema, List<Record> expectedRecords, int numRecord)
+ void writeAndValidate(
+ Schema schema,
+ List<org.apache.iceberg.data.Record> expectedRecords,
Review Comment:
Does it need to be fully qualified?
--
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