Skip to content
This repository was archived by the owner on Jul 23, 2024. It is now read-only.

HAWQ-1671. add compile support for orc formatter and wrapper#1399

Closed
ion2014 wants to merge 2 commits intoapache:masterfrom
ion2014:plug_orc
Closed

HAWQ-1671. add compile support for orc formatter and wrapper#1399
ion2014 wants to merge 2 commits intoapache:masterfrom
ion2014:plug_orc

Conversation

@ion2014
Copy link
Copy Markdown
Contributor

@ion2014 ion2014 commented Nov 4, 2018

Changed Makefile in contrib/orc
Add folder orc_formatter and orc_wrapper for split compilation
Add dummy code for testing the Makefile

@huor
Copy link
Copy Markdown
Contributor

huor commented Nov 5, 2018

Use "HAWQ-1671. add ..." instead of "[HAWQ-1671] add ..."

ifdef MAVEN
ifeq ($(shell java -version 2>&1 | grep 1.7.0 > /dev/null; printf $$?),0)
$(MAVEN) package -DskipTests -Dhttps.protocols=TLSv1.2
$(MAVEN) package -DskipTests -Dmaven.javadoc.skip=true -Dhttps.protocols=TLSv1.2
Copy link
Copy Markdown
Contributor

@huor huor Nov 5, 2018

Choose a reason for hiding this comment

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

Disable java doc pluggin?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is because the javadoc added doclint in jdk8 which required stricter code format (which is not followed by the former code), so I temporarily disabled the javadoc.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what's the reason of update hawq-hadoop/Makefile while this is a commit to add compile suppport for orc ?

Copy link
Copy Markdown
Contributor Author

@ion2014 ion2014 Nov 6, 2018

Choose a reason for hiding this comment

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

maybe I'll start a subtas for JDK8 support in another commit, thanks for your review!

#
MODULE_big = orc
OBJS = orc.o
OBJS = orc_wrapper/orc_wrapper.o orc_format/orc_format.o
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cwrapper/cwrapper.o format/format.o

Copy link
Copy Markdown
Member

@stanlyxiang stanlyxiang left a comment

Choose a reason for hiding this comment

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

please check all the comments and make sure the commit passed the jenkins and travis-ci check. The details link showed the details error message for your commit build.

ifdef MAVEN
ifeq ($(shell java -version 2>&1 | grep 1.7.0 > /dev/null; printf $$?),0)
$(MAVEN) package -DskipTests -Dhttps.protocols=TLSv1.2
$(MAVEN) package -DskipTests -Dmaven.javadoc.skip=true -Dhttps.protocols=TLSv1.2
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what's the reason of update hawq-hadoop/Makefile while this is a commit to add compile suppport for orc ?

@@ -0,0 +1 @@
//this is a test file for compiling. No newline at end of file
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if this file is useless, please do not add it to the commit.
if you use it as a placeholder, please add a more formal c source file with license header, otherwise your file won't pass apache-rat-check and no one will merge your commit to master.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the information, I'll add the license header on the amending commits.

<source>1.7</source>
<target>1.7</target>
<source>1.8</source>
<target>1.8</target>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what's the reason to update maven-compiler-plugin version for range-plugin ?

@ion2014 ion2014 force-pushed the plug_orc branch 3 times, most recently from 0c3949f to c6b0d89 Compare November 8, 2018 07:05
@ion2014 ion2014 changed the title [HAWQ-1671] add compile support for orc formatter and wrapper HAWQ-1671 add compile support for orc formatter and wrapper Nov 8, 2018
@ion2014 ion2014 changed the title HAWQ-1671 add compile support for orc formatter and wrapper HAWQ-1671. add compile support for orc formatter and wrapper Nov 8, 2018
@ion2014 ion2014 force-pushed the plug_orc branch 3 times, most recently from 240f07d to b094599 Compare November 8, 2018 07:34
	Changed Makefile in contrib/orc
	Add folder format and cwrapper for split compilation
	Add dummy code for testing the Makefile
@huor
Copy link
Copy Markdown
Contributor

huor commented Nov 8, 2018

+1

@jiny2
Copy link
Copy Markdown
Contributor

jiny2 commented Nov 8, 2018

+1 LGTM, Thanks.

	add cwrapper.cpp to generate interface code for C/C++ link.
	changed Makefile to support cpp compiling.
@ion2014 ion2014 closed this Nov 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants