-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
PARQUET-1153: Parquet-thrift doesn't compile with Thrift 0.10.0 #434
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, will merge it in a few days unless there are objections.
pom.xml
Outdated
@@ -82,7 +81,7 @@ | |||
<scala.maven.test.skip>false</scala.maven.test.skip> | |||
<pig.version>0.16.0</pig.version> | |||
<pig.classifier>h2</pig.classifier> | |||
<thrift.version>0.7.0</thrift.version> | |||
<thrift.version>0.9.3</thrift.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please also update the Thrift version in the exclude lines:
<exclude>thrift-0.7.0/**</exclude>
<exclude>thrift-0.7.0.tar.gz</exclude>
Replacing them with ${thrift.version} would be the best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.travis.yml will also have to be updated to use 0.9.3
.travis.yml
Outdated
- wget -nv http://archive.apache.org/dist/thrift/0.7.0/thrift-0.7.0.tar.gz | ||
- tar zxf thrift-0.7.0.tar.gz | ||
- cd thrift-0.7.0 | ||
- wget -nv http://archive.apache.org/dist/thrift/0.9.3/thrift-0.9.3.tar.gz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest the following:
THRIFT_VERSION=$(mvn -q -Dexec.executable=echo -Dexec.args='${thrift.version}' --non-recursive org.codehaus.mojo:exec-maven-plugin:1.3.1:exec)
wget -nv http://archive.apache.org/dist/thrift/${THRIFT_VERSION}/thrift-${THRIFT_VERSION}.tar.gz
tar zxf thrift-${THRIFT_VERSION}.tar.gz
cd thrift-${THRIFT_VERSION}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would have to move the before_install commands to a separate shell script to make this work, so we may not want to do this change at this time after all.
@zivanfi due to THRIFT-3419 thrift maven plugin is unusable for 0.9.3 release. I suggest upgrading just the plugin to 0.10.0, but leaving Thrift version at 0.7.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I merged your change.
No description provided.