You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Carl Steinbach <ca...@cloudera.com> on 2012/08/10 22:31:35 UTC
Re: Review Request: HIVE-3213: ODBC API enhancements
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5685/#review10147
-----------------------------------------------------------
Looks good overall. Most of things I commented on have to do with formatting issues and missing ASF license headers. It may be worth running all of this code through an automated formatter like astyle.
odbc/src/cpp/HiveResultSet.cpp
<https://reviews.apache.org/r/5685/#comment21496>
I think we should try to keep the formatting consistent.
* Always use braces for blocks (if, while, etc)
* open brace always on the same line
* "} else {"
odbc/src/cpp/HiveResultSet.cpp
<https://reviews.apache.org/r/5685/#comment21498>
Brace should after the closing paren.
odbc/src/cpp/hiveclient.def
<https://reviews.apache.org/r/5685/#comment21495>
Missing ASF license header.
odbc/src/cpp/hiveclienthelper.h
<https://reviews.apache.org/r/5685/#comment21494>
Formatting: please fix the line break alignment.
odbc/src/cpp/if/fb303.thrift
<https://reviews.apache.org/r/5685/#comment21491>
This file shouldn't be necessary.
odbc/src/cpp/if/hive_metastore.thrift
<https://reviews.apache.org/r/5685/#comment21492>
Please remove and use metastore/if/hive_metastore.thrift instead.
odbc/src/cpp/if/hive_service.thrift
<https://reviews.apache.org/r/5685/#comment21490>
Please remove this file and use the copy here instead: service/if/hive_service.thrift
odbc/src/cpp/if/queryplan.thrift
<https://reviews.apache.org/r/5685/#comment21489>
This file can be removed. We should instead reference the copy in ql/if/queryplan.thrift
odbc/src/driver/hiveodbc.h
<https://reviews.apache.org/r/5685/#comment21487>
Please remove the CDH reference in the comment and bump the version number to 0.10.0
odbc/src/driver/hiveodbc.h
<https://reviews.apache.org/r/5685/#comment21488>
Formatting: It would be nice if the comments for these structs were left-justified.
odbc/src/driver/hiveodbc.c
<https://reviews.apache.org/r/5685/#comment21501>
Is is possible to reformat this so we get closer to the 100 character line limit?
odbc/src/driver/hiveodbc.c
<https://reviews.apache.org/r/5685/#comment21502>
Formatting.
odbc/src/driver/hiveodbc.c
<https://reviews.apache.org/r/5685/#comment21503>
Remove.
odbc/src/driver/hiveodbc_win32_rc.rc
<https://reviews.apache.org/r/5685/#comment21505>
Missing ASF header.
odbc/src/driver/libhiveodbc.def
<https://reviews.apache.org/r/5685/#comment21506>
Missing ASF header.
- Carl Steinbach
On June 30, 2012, 4:38 a.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5685/
> -----------------------------------------------------------
>
> (Updated June 30, 2012, 4:38 a.m.)
>
>
> Review request for hive and Carl Steinbach.
>
>
> Description
> -------
>
> Enhanced ODBC driver with limited ODBC3 compliance. This ticket HIVE-3213 covers the source code, the build changes are tracked by HIVE-3212
>
>
> This addresses bug HIVE-3213.
> https://issues.apache.org/jira/browse/HIVE-3213
>
>
> Diffs
> -----
>
> odbc/src/cpp/HiveConnection.h 3b2e2b1
> odbc/src/cpp/HiveResultSet.h 25eabc4
> odbc/src/cpp/HiveResultSet.cpp d3d375e
> odbc/src/cpp/HiveRowSet.h ca6e6af
> odbc/src/cpp/HiveRowSet.cpp 3de6124
> odbc/src/cpp/Makefile.am PRE-CREATION
> odbc/src/cpp/Makefile.in PRE-CREATION
> odbc/src/cpp/hiveclient.h f1af670
> odbc/src/cpp/hiveclient.cpp 450eb0b
> odbc/src/cpp/hiveclient.def PRE-CREATION
> odbc/src/cpp/hiveclienthelper.h 5814a03
> odbc/src/cpp/hiveconstants.h 72f1049
> odbc/src/cpp/if/fb303.thrift PRE-CREATION
> odbc/src/cpp/if/hive_metastore.thrift PRE-CREATION
> odbc/src/cpp/if/hive_service.thrift PRE-CREATION
> odbc/src/cpp/if/queryplan.thrift PRE-CREATION
> odbc/src/cpp/thriftserverconstants.h fe4bac4
> odbc/src/driver/Makefile.am PRE-CREATION
> odbc/src/driver/Makefile.in PRE-CREATION
> odbc/src/driver/hiveodbc.h PRE-CREATION
> odbc/src/driver/hiveodbc.c PRE-CREATION
> odbc/src/driver/hiveodbc_logo.ico PRE-CREATION
> odbc/src/driver/hiveodbc_win32_rc.h PRE-CREATION
> odbc/src/driver/hiveodbc_win32_rc.rc PRE-CREATION
> odbc/src/driver/libhiveodbc.def PRE-CREATION
> odbc/src/driver/libtool-version PRE-CREATION
> odbc/src/test/Makefile.am PRE-CREATION
> odbc/src/test/Makefile.in PRE-CREATION
> odbc/src/test/hiveclienttest.c fbb4e24
> odbc/src/test/hiveodbctest.c PRE-CREATION
> odbc/src/test/hivetest.h PRE-CREATION
>
> Diff: https://reviews.apache.org/r/5685/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Prasad Mujumdar
>
>
Re: Review Request: HIVE-3213: ODBC API enhancements
Posted by Prasad Mujumdar <pr...@cloudera.com>.
> On Aug. 10, 2012, 8:31 p.m., Carl Steinbach wrote:
> > odbc/src/cpp/hiveclient.def, line 1
> > <https://reviews.apache.org/r/5685/diff/1/?file=117650#file117650line1>
> >
> > Missing ASF license header.
Done
> On Aug. 10, 2012, 8:31 p.m., Carl Steinbach wrote:
> > odbc/src/cpp/HiveResultSet.cpp, line 72
> > <https://reviews.apache.org/r/5685/diff/1/?file=117643#file117643line72>
> >
> > I think we should try to keep the formatting consistent.
> >
> > * Always use braces for blocks (if, while, etc)
> > * open brace always on the same line
> > * "} else {"
Done
> On Aug. 10, 2012, 8:31 p.m., Carl Steinbach wrote:
> > odbc/src/cpp/HiveResultSet.cpp, line 225
> > <https://reviews.apache.org/r/5685/diff/1/?file=117643#file117643line225>
> >
> > Brace should after the closing paren.
Done
> On Aug. 10, 2012, 8:31 p.m., Carl Steinbach wrote:
> > odbc/src/cpp/hiveclienthelper.h, line 91
> > <https://reviews.apache.org/r/5685/diff/1/?file=117651#file117651line91>
> >
> > Formatting: please fix the line break alignment.
Done
> On Aug. 10, 2012, 8:31 p.m., Carl Steinbach wrote:
> > odbc/src/cpp/if/fb303.thrift, line 1
> > <https://reviews.apache.org/r/5685/diff/1/?file=117653#file117653line1>
> >
> > This file shouldn't be necessary.
Looks like the fb303 is needed, so thats the only file I am keeping there. The makefile is updated Rest of access rest of thrift IDLs from other source directories.
> On Aug. 10, 2012, 8:31 p.m., Carl Steinbach wrote:
> > odbc/src/cpp/if/hive_metastore.thrift, line 1
> > <https://reviews.apache.org/r/5685/diff/1/?file=117654#file117654line1>
> >
> > Please remove and use metastore/if/hive_metastore.thrift instead.
Done
> On Aug. 10, 2012, 8:31 p.m., Carl Steinbach wrote:
> > odbc/src/cpp/if/queryplan.thrift, line 1
> > <https://reviews.apache.org/r/5685/diff/1/?file=117656#file117656line1>
> >
> > This file can be removed. We should instead reference the copy in ql/if/queryplan.thrift
Done
> On Aug. 10, 2012, 8:31 p.m., Carl Steinbach wrote:
> > odbc/src/driver/hiveodbc.h, line 131
> > <https://reviews.apache.org/r/5685/diff/1/?file=117660#file117660line131>
> >
> > Please remove the CDH reference in the comment and bump the version number to 0.10.0
Done
> On Aug. 10, 2012, 8:31 p.m., Carl Steinbach wrote:
> > odbc/src/driver/hiveodbc.c, line 8782
> > <https://reviews.apache.org/r/5685/diff/1/?file=117661#file117661line8782>
> >
> > Is is possible to reformat this so we get closer to the 100 character line limit?
Done
> On Aug. 10, 2012, 8:31 p.m., Carl Steinbach wrote:
> > odbc/src/driver/hiveodbc.c, line 8829
> > <https://reviews.apache.org/r/5685/diff/1/?file=117661#file117661line8829>
> >
> > Formatting.
Done
> On Aug. 10, 2012, 8:31 p.m., Carl Steinbach wrote:
> > odbc/src/driver/hiveodbc.c, line 11680
> > <https://reviews.apache.org/r/5685/diff/1/?file=117661#file117661line11680>
> >
> > Remove.
Done
> On Aug. 10, 2012, 8:31 p.m., Carl Steinbach wrote:
> > odbc/src/driver/hiveodbc_win32_rc.rc, line 1
> > <https://reviews.apache.org/r/5685/diff/1/?file=117664#file117664line1>
> >
> > Missing ASF header.
Added header.
> On Aug. 10, 2012, 8:31 p.m., Carl Steinbach wrote:
> > odbc/src/driver/libhiveodbc.def, line 1
> > <https://reviews.apache.org/r/5685/diff/1/?file=117665#file117665line1>
> >
> > Missing ASF header.
Added header
- Prasad
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5685/#review10147
-----------------------------------------------------------
On Oct. 8, 2012, 5:10 a.m., Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5685/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2012, 5:10 a.m.)
>
>
> Review request for hive and Carl Steinbach.
>
>
> Description
> -------
>
> Enhanced ODBC driver with limited ODBC3 compliance. This ticket HIVE-3213 covers the source code, the build changes are tracked by HIVE-3212
>
>
> This addresses bug HIVE-3213.
> https://issues.apache.org/jira/browse/HIVE-3213
>
>
> Diffs
> -----
>
> odbc/src/cpp/HiveConnection.h 3b2e2b1
> odbc/src/cpp/HiveResultSet.h 25eabc4
> odbc/src/cpp/HiveResultSet.cpp d3d375e
> odbc/src/cpp/HiveRowSet.h ca6e6af
> odbc/src/cpp/HiveRowSet.cpp 3de6124
> odbc/src/cpp/Makefile.am PRE-CREATION
> odbc/src/cpp/Makefile.in PRE-CREATION
> odbc/src/cpp/hiveclient.h f1af670
> odbc/src/cpp/hiveclient.cpp 450eb0b
> odbc/src/cpp/hiveclient.def PRE-CREATION
> odbc/src/cpp/hiveclienthelper.h 5814a03
> odbc/src/cpp/hiveconstants.h 72f1049
> odbc/src/cpp/if/fb303.thrift PRE-CREATION
> odbc/src/cpp/if/hive_metastore.thrift PRE-CREATION
> odbc/src/cpp/if/hive_service.thrift PRE-CREATION
> odbc/src/cpp/if/queryplan.thrift PRE-CREATION
> odbc/src/cpp/thriftserverconstants.h fe4bac4
> odbc/src/driver/Makefile.am PRE-CREATION
> odbc/src/driver/Makefile.in PRE-CREATION
> odbc/src/driver/hiveodbc.h PRE-CREATION
> odbc/src/driver/hiveodbc.c PRE-CREATION
> odbc/src/driver/hiveodbc_win32_rc.h PRE-CREATION
> odbc/src/driver/hiveodbc_win32_rc.rc PRE-CREATION
> odbc/src/driver/libhiveodbc.def PRE-CREATION
> odbc/src/test/Makefile.am PRE-CREATION
> odbc/src/test/Makefile.in PRE-CREATION
> odbc/src/test/hiveclienttest.c fbb4e24
> odbc/src/test/hiveodbctest.c PRE-CREATION
> odbc/src/test/hivetest.h PRE-CREATION
>
> Diff: https://reviews.apache.org/r/5685/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Prasad Mujumdar
>
>