Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Arrow, Parquet, Spark 3.5, Flink 1.20: Avoid deprecated method #11874

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ebyhr
Copy link
Contributor

@ebyhr ebyhr commented Dec 25, 2024

No description provided.

@github-actions github-actions bot added the spark label Dec 25, 2024
@@ -251,7 +251,8 @@ private String getCompressionType(InputFile inputFile) throws Exception {
return orcReader.getCompressionKind().name();
case PARQUET:
ParquetMetadata footer =
ParquetFileReader.readFooter(CONF, new Path(inputFile.location()), NO_FILTER);
Copy link
Contributor Author

@ebyhr ebyhr Dec 25, 2024

Choose a reason for hiding this comment

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

@@ -107,8 +107,8 @@ public void testAlterTable() throws NoSuchTableException {

assertThat(table).as("Should return updated table").isNotNull();

StructField expectedField = DataTypes.createStructField(fieldName, DataTypes.StringType, true);
assertThat(table.schema().fields()[2])
Copy link
Contributor Author

@ebyhr ebyhr Dec 25, 2024

Choose a reason for hiding this comment

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


} finally {
visitor.fieldNames.pop();
if (annotation.equals(LogicalTypeAnnotation.listType())) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would recommend reviewing this changer with "Hide whitespace" option.
I changed switch to if because LogicalTypeAnnotation isn't enum.

@@ -59,106 +59,101 @@ public static <T> T visit(DataType sType, Type type, ParquetWithSparkSchemaVisit
} else {
// if not a primitive, the typeId must be a group
GroupType group = type.asGroupType();
OriginalType annotation = group.getOriginalType();
Copy link
Contributor Author

@ebyhr ebyhr Dec 25, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

there are a few more places in the codebase that use this, so maybe those places should be updated as well as part of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated except for Spark 3.4 & 3.3 and Flink 1.19 & 1.18.

@ebyhr
Copy link
Contributor Author

ebyhr commented Dec 25, 2024

CI hit #11651


} finally {
visitor.fieldNames.pop();
if (annotation.equals(LogicalTypeAnnotation.listType())) {
Copy link
Contributor

@nastra nastra Jan 7, 2025

Choose a reason for hiding this comment

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

it would be better if the check is the other way around, because the annotation could be null: LogicalTypeAnnotation.listType().equals(annotation).
That way we could save ourselves one nesting and remove the if (annotation != null) block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This block is inside of annotation != null. Do you want to remove the outer if?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, let's remove that if block

@ebyhr ebyhr changed the title Spark 3.5: Avoid deprecated method Arrow, Parquet, Spark 3.5, Flink 1.20: Avoid deprecated method Jan 8, 2025
@nastra nastra requested review from pvary and Fokko January 8, 2025 10:14
Copy link
Contributor

@pvary pvary left a comment

Choose a reason for hiding this comment

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

Flink part LGTM
Please don't forget to backport it to Flink 1.18/1.19 after merging this PR.
Thanks,
Peter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants