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.

Sign up for GitHub

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

Jump to bottom

ORC-1327: Exclude the proto files from the nohive jar #1334

Closed
wants to merge 1 commit into from
Closed

ORC-1327: Exclude the proto files from the nohive jar #1334

wants to merge 1 commit into from

Conversation

Fokko
Copy link
Contributor

@Fokko Fokko commented Dec 6, 2022

Why are the changes needed?

We're seeing some conflicts in the proto files when updating Iceberg in Trino: trinodb/trino#15079 (comment)

I think we should exclude the proto files from the nohive jar.

Before the change:

➜  java git:(main) ✗ rm -rf core/target/
➜  java git:(main) ✗ mvn package -DskipTests
➜  java git:(main) ✗ cd core/target
➜  target git:(main) ✗ unzip orc-core-1.9.0-SNAPSHOT-nohive.jar
➜  target git:(main) find . | grep -i "google/protobuf/"
./google/protobuf/timestamp.proto
./google/protobuf/field_mask.proto
./google/protobuf/api.proto
./google/protobuf/duration.proto
./google/protobuf/struct.proto
./google/protobuf/wrappers.proto
./google/protobuf/source_context.proto
./google/protobuf/any.proto
./google/protobuf/type.proto
./google/protobuf/empty.proto
./google/protobuf/compiler
./google/protobuf/compiler/plugin.proto
./google/protobuf/descriptor.proto

After the change:

➜  java git:(main) ✗ rm -rf core/target/
➜  java git:(main) ✗ mvn package -DskipTests
➜  java git:(main) ✗ cd core/target
➜  target git:(main) ✗ unzip orc-core-1.9.0-SNAPSHOT-nohive.jar
➜  target git:(main) ✗ find . | grep -i "google/protobuf/"
∅

Interestingly enough, the normal jar doesn't contain these proto files:

➜  java git:(main) ✗ rm -rf core/target/
➜  java git:(main) ✗ mvn package -DskipTests
➜  java git:(main) ✗ cd core/target
➜  target git:(main) unzip orc-core-1.9.0-SNAPSHOT.jar| grep -i proto
➜  target git:(main) find . | grep -i "google/protobuf/"
∅

What changes were proposed in this pull request?

Exclude the proto files

How was this patch tested?

Inspecting the produced jar

Closes #1333

@Fokko
ORC-1333: Exclude the proto files from nohive
475d9fd 
Before the change:
```
➜  java git:(main) ✗ rm -rf core/target/
➜  java git:(main) ✗ mvn package -DskipTests
➜  java git:(main) ✗ cd core/target
➜  target git:(main) ✗ unzip orc-core-1.9.0-SNAPSHOT-nohive.jar
➜  target git:(main) find . | grep -i "google/protobuf/"
./google/protobuf/timestamp.proto
./google/protobuf/field_mask.proto
./google/protobuf/api.proto
./google/protobuf/duration.proto
./google/protobuf/struct.proto
./google/protobuf/wrappers.proto
./google/protobuf/source_context.proto
./google/protobuf/any.proto
./google/protobuf/type.proto
./google/protobuf/empty.proto
./google/protobuf/compiler
./google/protobuf/compiler/plugin.proto
./google/protobuf/descriptor.proto
```

After the change:
```
➜  java git:(main) ✗ rm -rf core/target/
➜  java git:(main) ✗ mvn package -DskipTests
➜  java git:(main) ✗ cd core/target
➜  target git:(main) ✗ unzip orc-core-1.9.0-SNAPSHOT-nohive.jar
➜  target git:(main) ✗ find . | grep -i "google/protobuf/"
∅
```

Interestingly enough, the normal jar doesn't contain these proto files:
```
➜  java git:(main) ✗ rm -rf core/target/
➜  java git:(main) ✗ mvn package -DskipTests
➜  java git:(main) ✗ cd core/target
➜  target git:(main) unzip orc-core-1.9.0-SNAPSHOT.jar| grep -i proto
➜  target git:(main) find . | grep -i "google/protobuf/"
∅
```
@dongjoon-hyun dongjoon-hyun added this to the 1.8.2 milestone Dec 6, 2022
dongjoon-hyun
dongjoon-hyun reviewed Dec 6, 2022
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for making a PR, @Fokko .
According to the attached Trino PR, this will be Apache ORC 1.8.2 because Aapche Iceberg 1.1.0 is using Apache ORC 1.8.0.

@Fokko Fokko mentioned this pull request Dec 6, 2022
Merged
@Fokko
Copy link
Contributor Author

Fokko commented Dec 6, 2022

Thanks @dongjoon-hyun that would be great! 🙏🏻

@dongjoon-hyun
Copy link
Member

BTW, @Fokko . We recently released Apache ORC 1.8.1. So, 1.8.2 is scheduled on March 03, 2023 (https://github.com/apache/orc/milestone/15). Is it okay?

dongjoon-hyun
dongjoon-hyun approved these changes Dec 6, 2022
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM.

dongjoon-hyun pushed a commit that referenced this pull request Dec 6, 2022
@Fokko @dongjoon-hyun
ORC-1333: Exclude the proto files from the nohive jar
d49cc2d 
### Why are the changes needed?

We're seeing some conflicts in the proto files when updating Iceberg in Trino: trinodb/trino#15079 (comment)

I think we should exclude the proto files from the `nohive` jar.

Before the change:
```
➜  java git:(main) ✗ rm -rf core/target/
➜  java git:(main) ✗ mvn package -DskipTests
➜  java git:(main) ✗ cd core/target
➜  target git:(main) ✗ unzip orc-core-1.9.0-SNAPSHOT-nohive.jar
➜  target git:(main) find . | grep -i "google/protobuf/"
./google/protobuf/timestamp.proto
./google/protobuf/field_mask.proto
./google/protobuf/api.proto
./google/protobuf/duration.proto
./google/protobuf/struct.proto
./google/protobuf/wrappers.proto
./google/protobuf/source_context.proto
./google/protobuf/any.proto
./google/protobuf/type.proto
./google/protobuf/empty.proto
./google/protobuf/compiler
./google/protobuf/compiler/plugin.proto
./google/protobuf/descriptor.proto
```

After the change:
```
➜  java git:(main) ✗ rm -rf core/target/
➜  java git:(main) ✗ mvn package -DskipTests
➜  java git:(main) ✗ cd core/target
➜  target git:(main) ✗ unzip orc-core-1.9.0-SNAPSHOT-nohive.jar
➜  target git:(main) ✗ find . | grep -i "google/protobuf/"
∅
```

Interestingly enough, the normal jar doesn't contain these proto files:
```
➜  java git:(main) ✗ rm -rf core/target/
➜  java git:(main) ✗ mvn package -DskipTests
➜  java git:(main) ✗ cd core/target
➜  target git:(main) unzip orc-core-1.9.0-SNAPSHOT.jar| grep -i proto
➜  target git:(main) find . | grep -i "google/protobuf/"
∅
```

### What changes were proposed in this pull request?

Exclude the proto files

### How was this patch tested?

Inspecting the produced jar

Closes #1333

Closes #1334 from Fokko/fd-exclude-proto-files.

Authored-by: Fokko Driesprong <fokko@tabular.io>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
(cherry picked from commit f6fee07)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
@Fokko Fokko deleted the fd-exclude-proto-files branch December 6, 2022 18:15
@Fokko
Copy link
Contributor Author

Fokko commented Dec 6, 2022

@dongjoon-hyun Thanks. It depends, if Trino is fine with excluding the proto files then March next year would be perfectly fine. Otherwise, it would be great to fast-track it if possible.

@dongjoon-hyun
Copy link
Member

Oh, @Fokko , what is ORC-1333? It's not Apache ORC JIRA ID.

@Fokko
Copy link
Contributor Author

Fokko commented Dec 6, 2022

@dongjoon-hyun it is the ID of the GitHub Issue: #1333

@dongjoon-hyun
Copy link
Member

My bad. Let me revert this. Unfortunately, it's not Apache ORC way, @Fokko .

@dongjoon-hyun
Copy link
Member

It's reverted from main/branch-1.8.

@dongjoon-hyun
Copy link
Member

I'm working on ORC JIRA and recommitting, @Fokko . There is nothing to do from your side~

dongjoon-hyun pushed a commit that referenced this pull request Dec 6, 2022
@Fokko @dongjoon-hyun
ORC-1327: Exclude the proto files from the nohive jar
5ea8c06 
We're seeing some conflicts in the proto files when updating Iceberg in Trino: trinodb/trino#15079 (comment)

I think we should exclude the proto files from the `nohive` jar.

Before the change:
```
➜  java git:(main) ✗ rm -rf core/target/
➜  java git:(main) ✗ mvn package -DskipTests
➜  java git:(main) ✗ cd core/target
➜  target git:(main) ✗ unzip orc-core-1.9.0-SNAPSHOT-nohive.jar
➜  target git:(main) find . | grep -i "google/protobuf/"
./google/protobuf/timestamp.proto
./google/protobuf/field_mask.proto
./google/protobuf/api.proto
./google/protobuf/duration.proto
./google/protobuf/struct.proto
./google/protobuf/wrappers.proto
./google/protobuf/source_context.proto
./google/protobuf/any.proto
./google/protobuf/type.proto
./google/protobuf/empty.proto
./google/protobuf/compiler
./google/protobuf/compiler/plugin.proto
./google/protobuf/descriptor.proto
```

After the change:
```
➜  java git:(main) ✗ rm -rf core/target/
➜  java git:(main) ✗ mvn package -DskipTests
➜  java git:(main) ✗ cd core/target
➜  target git:(main) ✗ unzip orc-core-1.9.0-SNAPSHOT-nohive.jar
➜  target git:(main) ✗ find . | grep -i "google/protobuf/"
∅
```

Interestingly enough, the normal jar doesn't contain these proto files:
```
➜  java git:(main) ✗ rm -rf core/target/
➜  java git:(main) ✗ mvn package -DskipTests
➜  java git:(main) ✗ cd core/target
➜  target git:(main) unzip orc-core-1.9.0-SNAPSHOT.jar| grep -i proto
➜  target git:(main) find . | grep -i "google/protobuf/"
∅
```

Exclude the proto files

Inspecting the produced jar

Closes #1333

Closes #1334 from Fokko/fd-exclude-proto-files.

Authored-by: Fokko Driesprong <fokko@tabular.io>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
(cherry picked from commit f6fee07)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
dongjoon-hyun pushed a commit that referenced this pull request Dec 6, 2022
@Fokko @dongjoon-hyun
ORC-1327: Exclude the proto files from the nohive jar
7bfdaaf 
We're seeing some conflicts in the proto files when updating Iceberg in Trino: trinodb/trino#15079 (comment)

I think we should exclude the proto files from the `nohive` jar.

Before the change:
```
➜  java git:(main) ✗ rm -rf core/target/
➜  java git:(main) ✗ mvn package -DskipTests
➜  java git:(main) ✗ cd core/target
➜  target git:(main) ✗ unzip orc-core-1.9.0-SNAPSHOT-nohive.jar
➜  target git:(main) find . | grep -i "google/protobuf/"
./google/protobuf/timestamp.proto
./google/protobuf/field_mask.proto
./google/protobuf/api.proto
./google/protobuf/duration.proto
./google/protobuf/struct.proto
./google/protobuf/wrappers.proto
./google/protobuf/source_context.proto
./google/protobuf/any.proto
./google/protobuf/type.proto
./google/protobuf/empty.proto
./google/protobuf/compiler
./google/protobuf/compiler/plugin.proto
./google/protobuf/descriptor.proto
```

After the change:
```
➜  java git:(main) ✗ rm -rf core/target/
➜  java git:(main) ✗ mvn package -DskipTests
➜  java git:(main) ✗ cd core/target
➜  target git:(main) ✗ unzip orc-core-1.9.0-SNAPSHOT-nohive.jar
➜  target git:(main) ✗ find . | grep -i "google/protobuf/"
∅
```

Interestingly enough, the normal jar doesn't contain these proto files:
```
➜  java git:(main) ✗ rm -rf core/target/
➜  java git:(main) ✗ mvn package -DskipTests
➜  java git:(main) ✗ cd core/target
➜  target git:(main) unzip orc-core-1.9.0-SNAPSHOT.jar| grep -i proto
➜  target git:(main) find . | grep -i "google/protobuf/"
∅
```

Exclude the proto files

Inspecting the produced jar

Closes #1333

Closes #1334 from Fokko/fd-exclude-proto-files.

Authored-by: Fokko Driesprong <fokko@tabular.io>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
(cherry picked from commit f6fee07)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
(cherry picked from commit 5ea8c06)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
@Fokko
Copy link
Contributor Author

Fokko commented Dec 6, 2022

@dongjoon-hyun Ah, race condition. I created a new PR: #1335

@dongjoon-hyun dongjoon-hyun changed the title ORC-1333: Exclude the proto files from the nohive jar ORC-1327: Exclude the proto files from the nohive jar Dec 6, 2022
@dongjoon-hyun
Copy link
Member

Oh, @Fokko . I was handling it by myself as I mentioned #1334 (comment)

@nastra nastra mentioned this pull request Dec 7, 2022
Closed
dongjoon-hyun pushed a commit that referenced this pull request Dec 7, 2022
@nastra @dongjoon-hyun
ORC-1328: Exclude the proto files from the shaded protobuf jar
72deb94 
### Why are the changes needed?

It looks like the shaded-protobuf.jar has the same issue that was fixed in #1334

### What changes were proposed in this pull request?

Exclude the proto files

### How was this patch tested?

Test manually.

Closes #1336 from nastra/orc-1327.

Authored-by: Eduard Tudenhoefner <etudenhoefner@gmail.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
dongjoon-hyun pushed a commit that referenced this pull request Dec 7, 2022
@nastra @dongjoon-hyun
ORC-1328: Exclude the proto files from the shaded protobuf jar
71bd145 
### Why are the changes needed?

It looks like the shaded-protobuf.jar has the same issue that was fixed in #1334

### What changes were proposed in this pull request?

Exclude the proto files

### How was this patch tested?

Test manually.

Closes #1336 from nastra/orc-1327.

Authored-by: Eduard Tudenhoefner <etudenhoefner@gmail.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
(cherry picked from commit 72deb94)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
@Fokko Fokko mentioned this pull request Dec 12, 2022
Closed
cxzl25 pushed a commit to cxzl25/orc that referenced this pull request Jan 11, 2024
@Fokko @cxzl25
ORC-1333: Exclude the proto files from the nohive jar
0d1630f 
### Why are the changes needed?

We're seeing some conflicts in the proto files when updating Iceberg in Trino: trinodb/trino#15079 (comment)

I think we should exclude the proto files from the `nohive` jar.

Before the change:
```
➜  java git:(main) ✗ rm -rf core/target/
➜  java git:(main) ✗ mvn package -DskipTests
➜  java git:(main) ✗ cd core/target
➜  target git:(main) ✗ unzip orc-core-1.9.0-SNAPSHOT-nohive.jar
➜  target git:(main) find . | grep -i "google/protobuf/"
./google/protobuf/timestamp.proto
./google/protobuf/field_mask.proto
./google/protobuf/api.proto
./google/protobuf/duration.proto
./google/protobuf/struct.proto
./google/protobuf/wrappers.proto
./google/protobuf/source_context.proto
./google/protobuf/any.proto
./google/protobuf/type.proto
./google/protobuf/empty.proto
./google/protobuf/compiler
./google/protobuf/compiler/plugin.proto
./google/protobuf/descriptor.proto
```

After the change:
```
➜  java git:(main) ✗ rm -rf core/target/
➜  java git:(main) ✗ mvn package -DskipTests
➜  java git:(main) ✗ cd core/target
➜  target git:(main) ✗ unzip orc-core-1.9.0-SNAPSHOT-nohive.jar
➜  target git:(main) ✗ find . | grep -i "google/protobuf/"
∅
```

Interestingly enough, the normal jar doesn't contain these proto files:
```
➜  java git:(main) ✗ rm -rf core/target/
➜  java git:(main) ✗ mvn package -DskipTests
➜  java git:(main) ✗ cd core/target
➜  target git:(main) unzip orc-core-1.9.0-SNAPSHOT.jar| grep -i proto
➜  target git:(main) find . | grep -i "google/protobuf/"
∅
```

### What changes were proposed in this pull request?

Exclude the proto files

### How was this patch tested?

Inspecting the produced jar

Closes apache#1333

Closes apache#1334 from Fokko/fd-exclude-proto-files.

Authored-by: Fokko Driesprong <fokko@tabular.io>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
cxzl25 pushed a commit to cxzl25/orc that referenced this pull request Jan 11, 2024
@Fokko @cxzl25
ORC-1327: Exclude the proto files from the nohive jar
263d41e 
We're seeing some conflicts in the proto files when updating Iceberg in Trino: trinodb/trino#15079 (comment)

I think we should exclude the proto files from the `nohive` jar.

Before the change:
```
➜  java git:(main) ✗ rm -rf core/target/
➜  java git:(main) ✗ mvn package -DskipTests
➜  java git:(main) ✗ cd core/target
➜  target git:(main) ✗ unzip orc-core-1.9.0-SNAPSHOT-nohive.jar
➜  target git:(main) find . | grep -i "google/protobuf/"
./google/protobuf/timestamp.proto
./google/protobuf/field_mask.proto
./google/protobuf/api.proto
./google/protobuf/duration.proto
./google/protobuf/struct.proto
./google/protobuf/wrappers.proto
./google/protobuf/source_context.proto
./google/protobuf/any.proto
./google/protobuf/type.proto
./google/protobuf/empty.proto
./google/protobuf/compiler
./google/protobuf/compiler/plugin.proto
./google/protobuf/descriptor.proto
```

After the change:
```
➜  java git:(main) ✗ rm -rf core/target/
➜  java git:(main) ✗ mvn package -DskipTests
➜  java git:(main) ✗ cd core/target
➜  target git:(main) ✗ unzip orc-core-1.9.0-SNAPSHOT-nohive.jar
➜  target git:(main) ✗ find . | grep -i "google/protobuf/"
∅
```

Interestingly enough, the normal jar doesn't contain these proto files:
```
➜  java git:(main) ✗ rm -rf core/target/
➜  java git:(main) ✗ mvn package -DskipTests
➜  java git:(main) ✗ cd core/target
➜  target git:(main) unzip orc-core-1.9.0-SNAPSHOT.jar| grep -i proto
➜  target git:(main) find . | grep -i "google/protobuf/"
∅
```

Exclude the proto files

Inspecting the produced jar

Closes apache#1333

Closes apache#1334 from Fokko/fd-exclude-proto-files.

Authored-by: Fokko Driesprong <fokko@tabular.io>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
(cherry picked from commit f6fee07)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
cxzl25 pushed a commit to cxzl25/orc that referenced this pull request Jan 11, 2024
@nastra @cxzl25
ORC-1328: Exclude the proto files from the shaded protobuf jar
66c272e 
### Why are the changes needed?

It looks like the shaded-protobuf.jar has the same issue that was fixed in apache#1334

### What changes were proposed in this pull request?

Exclude the proto files

### How was this patch tested?

Test manually.

Closes apache#1336 from nastra/orc-1327.

Authored-by: Eduard Tudenhoefner <etudenhoefner@gmail.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dongjoon-hyun dongjoon-hyun dongjoon-hyun approved these changes

Assignees
No one assigned
Labels
BUILD JAVA
Projects
None yet
Milestone
1.8.2
Development

Successfully merging this pull request may close these issues.

Exclude proto files from nohive
2 participants
@Fokko @dongjoon-hyun