[FLINK-27805][Connectors/ORC] bump orc version to 1.7.8 #22481

pgaref · 2023-04-25T03:56:39Z

https://issues.apache.org/jira/browse/FLINK-27805

Apache ORC 1.5.x is EOL -- last HF release happened on Sep 2021 https://orc.apache.org/news/2021/09/15/ORC-1.5.13/

Need to bump to 1.7.x -- Release Notes
ORC now supports writers with FSDataOutputStream (instead of just paths previously) so cleaning NoHivePhysicalWriterImpl and PhysicalWriterImpl

flinkbot · 2023-04-25T04:04:04Z

CI report:

361bdc2 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

MartijnVisser · 2023-04-25T09:55:00Z

@lirui-apache @JingsongLi Does one of you want to review this PR?

pgaref · 2023-04-25T22:41:16Z

Aslo cc @pnowojski / @akalash that might be interested

dmvk · 2023-05-03T09:52:25Z

Please add @liujiawinds as a co-author [1] to the commit.

[1] https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors

dmvk · 2023-05-03T09:53:28Z

flink-formats/flink-orc-nohive/pom.xml

+			<exclusions>
+				<exclusion>
+					<groupId>ch.qos.reload4j</groupId>
+					<artifactId>reload4j</artifactId>
+				</exclusion>
+				<exclusion>
+					<groupId>org.slf4j</groupId>
+					<artifactId>slf4j-reload4j</artifactId>
+				</exclusion>
+			</exclusions>


Why do we need these excludes?

Thats because we dont allow Reload4J dependencies due to their conflict with Log4j -- we use maven-enforcer [rules] for that (

flink/pom.xml

Line 1773 in 85efa13

<message>Log4j 1 and Reload4J dependencies are not allowed because they conflict with Log4j 2. If the dependency absolutely requires the Log4j 1 API, use 'org.apache.logging.log4j:log4j-1.2-api'.</message>

):

...rc-nohive/src/test/java/org/apache/flink/orc/nohive/OrcColumnarRowSplitReaderNoHiveTest.java

flink-formats/flink-orc/src/test/java/org/apache/flink/orc/OrcColumnarRowSplitReaderTest.java

flink-formats/flink-orc/src/test/java/org/apache/flink/orc/util/OrcBulkWriterTestUtil.java

dmvk · 2023-05-03T10:05:07Z

...k-formats/flink-orc/src/main/java/org/apache/flink/orc/writer/NoCloseFSDataOutputStream.java

+        // Don't close the internal stream here to avoid
+        // Stream Closed or ClosedChannelException when Flink performs checkpoint.


is this tested in any way? how do we close files then to avoid leaking resources?

I have the same question - although it looks like the original customized PhysicalWriterImpl does the same thing

Exactly, its the same functionality -- looks like we need the stream open for snapshotting, that is then cleaned up as part of snapshotContext.closeExceptionally method

I also replicated the ClosedChannelException issue described above when keeping the stream open in the existing tests so I believe we are good here

PS: we also do the same for other formats, e.g., Avro

dongjoon-hyun · 2023-05-03T15:43:49Z

Thank you, @pgaref and @dmvk .

cc @williamhyun

tzulitai · 2023-05-03T23:32:36Z

flink-formats/flink-orc/src/main/java/org/apache/flink/orc/writer/PhysicalWriterImpl.java

- * <p>NOTE: If the ORC dependency version is updated, this file may have to be updated as well to be
- * in sync with the new version's PhysicalFsWriter.


@pgaref quick question before I dive deeper:

I assume that the new PhysicalFsWriter using provided FSDataOutputStream has the exact same functionality as what was implemented with the original custom PhysicalWriterImpl and NoHivePhysicalWriterImpl? I did not do a line-by-line cross check, but for example, this Javadoc in the original PhysicalWriterImpl has me wondering.

Hey @tzulitai, thats correct the original (removed) PhysicalWriterImpl was a copy of the ORC PhysicalWriter with support for FSDataOutputStream. https://github.com/apache/orc/blob/a85b4c8852a894a701ddb73c15fb84ed1035abb9/java/core/src/java/org/apache/orc/impl/PhysicalFsWriter.java

ORC-1198 recently introduced a PhysicalFsWriter constructor with FSDataOutputStream as a parameter and there is no need to internally maintain this anymore 🥳

pgaref · 2023-05-04T16:37:13Z

@flinkbot run azure

dongjoon-hyun · 2023-05-04T16:47:17Z

BTW, @pgaref . As you know, @wgtmac started Apache ORC 1.7.9 RC1. Can we test RC1 in this PR?

Release Apache ORC 1.7.9 orc#1437

dongjoon-hyun · 2023-05-04T16:48:25Z

If we need some patches in order to help this PR, Apache ORC community can cut RC2 accordingly to help Apache Flink community.

pgaref · 2023-05-04T20:09:56Z

hey @dongjoon-hyun -- thanks for keeping an eye!
Just triggered a run on 1.7.9-SNAPSHOT

wgtmac · 2023-05-05T01:28:40Z

Thanks @dongjoon-hyun and @pgaref. I will keep an eye on it and prepare RC2 if required.

BTW, it would be better to test 1.7.9-RC1 instead of 1.7.9-SNAPSHOT. But I think they have the same content except the version.

pgaref · 2023-05-05T20:50:46Z

Thanks @dongjoon-hyun and @pgaref. I will keep an eye on it and prepare RC2 if required.

BTW, it would be better to test 1.7.9-RC1 instead of 1.7.9-SNAPSHOT. But I think they have the same content except the version.

FYI @dongjoon-hyun @wgtmac we had a green run for 1.7.9.

Switching back to 1.7.8 to get this PR merged and I will create a new ticket for the 1.7.9 bump when its ready.

dongjoon-hyun · 2023-05-05T20:51:30Z

It's great! Thank you so much, @pgaref .

dongjoon-hyun · 2023-05-05T20:52:05Z

BTW, could you cast your +1 with that information on Apache ORC 1.7.9 vote thread?

pgaref · 2023-05-05T20:52:22Z

BTW, could you cast your +1 with that information on Apache ORC 1.7.9 vote thread?

Sure, will do

dongjoon-hyun · 2023-05-05T20:53:16Z

Also, cc @williamhyun once more too..

Co-authored-by: Jia Liu <[email protected]>

pgaref · 2023-05-05T21:07:55Z

@tzulitai / @dmvk can you please take another look? I think we are almost there

tzulitai

Code changes looks good. To summarize what I gathered from understanding the PR changes:

Main change: previously custom maintained NoHivePhysicalWriterImpl and PhysicalWriterImpl was removed, in favor of the new PhysicalFsWriter that takes a FSDataOutputStream for instantiation. The code in the old PhysicalWriterImpl was already a copy of ORC's PhysicalFsWriter (with the addition of accepting a FSDataOutputStream), so we are not loosing any functional features or have any behavioral changes with this PR.
Test change # 1: renamed all f* field names to be _col* due to naming convention change in ORC 1.7.x.
Test change # : extended the ORC writer / reader tests to cover the new compression schemes in the new ORC version.

I'm slightly worried about 2) specifically, i.e. this field name convention change in the ORC upgrade. Would it cause any compatibility issues for Flink?

Context:

Flink filesystem sinks use 2PC to write with exactly-once guarantees.
This means that a Flink savepoint/checkpoint may contain staged "pre-committed" files with ORC format waiting to be committed, if the Flink job uses a sink that writes using ORC.
When restoring from that savepoint, those pre-commit files will be resumed from.

So, imagine if the savepoint was taken with a Flink version that was using ORC 1.5.x, but then the savepoint was restored using a Flink version that was using ORC 1.7.x. Would that be an issue with the field naming convention changes?

github-actions · 2025-01-16T12:06:26Z

This PR is being marked as stale since it has not had any activity in the last 180 days.
If you would like to keep this PR alive, please leave a comment asking for a review.
If the PR has merge conflicts, update it with the latest from the base branch.

If you are having difficulty finding a reviewer, please reach out to the
community, contact details can be found here: https://flink.apache.org/what-is-flink/community/

If this PR is no longer valid or desired, please feel free to close it.
If no activity occurs in the next 90 days, it will be automatically closed.

github-actions · 2025-04-03T06:26:02Z

This PR has been closed since it has not had any activity in 120 days.
If you feel like this was a mistake, or you would like to continue working on it,
please feel free to re-open the PR and ask for a review.

flinkbot added the component=Formats label Apr 25, 2023

pgaref force-pushed the FLINK-27805 branch from 57f6d75 to 323f377 Compare April 25, 2023 15:48

healchow approved these changes May 2, 2023

View reviewed changes

dmvk mentioned this pull request May 3, 2023

[FLINK-27805][Connectors/ORC] bump orc version to 1.7.5 #19844

Closed

dmvk reviewed May 3, 2023

View reviewed changes

tzulitai reviewed May 3, 2023

View reviewed changes

pgaref changed the title ~~[FLINK-27805] bump orc version to 1.7.8~~ [FLINK-27805][Connectors/ORC] bump orc version to 1.7.8 May 4, 2023

pgaref force-pushed the FLINK-27805 branch 3 times, most recently from 85efa13 to 446bb61 Compare May 4, 2023 06:10

pgaref force-pushed the FLINK-27805 branch from 446bb61 to 33eb0eb Compare May 4, 2023 20:06

pgaref force-pushed the FLINK-27805 branch from 33eb0eb to 278a952 Compare May 4, 2023 23:52

pgaref force-pushed the FLINK-27805 branch 2 times, most recently from ed0f291 to e4d2d89 Compare May 5, 2023 06:04

[FLINK-27805][Connectors/ORC] bump orc version to 1.7.8

361bdc2

Co-authored-by: Jia Liu <[email protected]>

pgaref force-pushed the FLINK-27805 branch from e4d2d89 to 361bdc2 Compare May 5, 2023 20:55

tzulitai reviewed May 9, 2023

View reviewed changes

github-actions bot added the stale label Jan 16, 2025

github-actions bot added the closed-stale label Apr 3, 2025

github-actions bot closed this Apr 3, 2025

		// Don't close the internal stream here to avoid
		// Stream Closed or ClosedChannelException when Flink performs checkpoint.

		* <p>NOTE: If the ORC dependency version is updated, this file may have to be updated as well to be
		* in sync with the new version's PhysicalFsWriter.

[FLINK-27805][Connectors/ORC] bump orc version to 1.7.8 #22481

[FLINK-27805][Connectors/ORC] bump orc version to 1.7.8 #22481

Uh oh!

Conversation

pgaref commented Apr 25, 2023 • edited by dmvk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flinkbot commented Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

MartijnVisser commented Apr 25, 2023

Uh oh!

pgaref commented Apr 25, 2023

Uh oh!

dmvk commented May 3, 2023

Uh oh!

dmvk May 3, 2023

Choose a reason for hiding this comment

Uh oh!

pgaref May 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dmvk May 3, 2023

Choose a reason for hiding this comment

Uh oh!

tzulitai May 3, 2023

Choose a reason for hiding this comment

Uh oh!

pgaref May 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented May 3, 2023

Uh oh!

tzulitai May 3, 2023

Choose a reason for hiding this comment

Uh oh!

pgaref May 4, 2023

Choose a reason for hiding this comment

Uh oh!

pgaref commented May 4, 2023

Uh oh!

dongjoon-hyun commented May 4, 2023

Uh oh!

dongjoon-hyun commented May 4, 2023

Uh oh!

pgaref commented May 4, 2023

Uh oh!

wgtmac commented May 5, 2023

Uh oh!

pgaref commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented May 5, 2023

Uh oh!

dongjoon-hyun commented May 5, 2023

Uh oh!

pgaref commented May 5, 2023

Uh oh!

dongjoon-hyun commented May 5, 2023

Uh oh!

pgaref commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tzulitai left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 16, 2025

Uh oh!

github-actions bot commented Apr 3, 2025

Uh oh!

Reviewers

Assignees

Labels

pgaref commented Apr 25, 2023 •

edited by dmvk

Loading

flinkbot commented Apr 25, 2023 •

edited

Loading

pgaref May 4, 2023 •

edited

Loading

pgaref May 4, 2023 •

edited

Loading

pgaref commented May 5, 2023 •

edited

Loading

pgaref commented May 5, 2023 •

edited

Loading