[
https://issues.apache.org/jira/browse/HCATALOG-241?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13209278#comment-13209278
]
[email protected] commented on HCATALOG-241:
--------------------------------------------------------
bq. On 2012-02-16 03:26:37, Francis Liu wrote:
bq. > /trunk/src/java/org/apache/hcatalog/common/HCatUtil.java, line 529
bq. > <https://reviews.apache.org/r/3819/diff/3/?file=75355#file75355line529>
bq. >
bq. > this is only used in tests. Do we really need this in produciton
code?
Agreed. Moving it to HCatDataCheckUtil in test.
bq. On 2012-02-16 03:26:37, Francis Liu wrote:
bq. > /trunk/src/java/org/apache/hcatalog/common/HCatUtil.java, line 553
bq. > <https://reviews.apache.org/r/3819/diff/3/?file=75355#file75355line553>
bq. >
bq. > Nit: You're throwing away the serDe once you get the
ObjectInspector. Just thinking you might change this to getSerDe instead and
get the object inspector from them. That way you can reuse the serDe if need be.
bq. >
bq. > Just noticed this isn't used anywhere in this patch. Is this really
used?
This was intended to be a utility method for Vikram to use. We'll
refactor/clean up if need be after that patch. But we don't really need the
serde after we get an OI if we cache the OI, and it makes sense to hold on to
the OI from the recordreader.
bq. On 2012-02-16 03:26:37, Francis Liu wrote:
bq. > /trunk/src/java/org/apache/hcatalog/common/HCatUtil.java, line 560
bq. > <https://reviews.apache.org/r/3819/diff/3/?file=75355#file75355line560>
bq. >
bq. > Nit: Same here..
This function should probably go away after Vikram's patch. We thought we
needed it initially, and then decided against it.
bq. On 2012-02-16 03:26:37, Francis Liu wrote:
bq. > /trunk/src/java/org/apache/hcatalog/data/HCatRecord.java, line 138
bq. > <https://reviews.apache.org/r/3819/diff/3/?file=75357#file75357line138>
bq. >
bq. > no longer needed, remove.
Agreed, removing commented code.
bq. On 2012-02-16 03:26:37, Francis Liu wrote:
bq. > /trunk/src/java/org/apache/hcatalog/data/HCatRecordObjectInspector.java,
line 43
bq. > <https://reviews.apache.org/r/3819/diff/3/?file=75358#file75358line43>
bq. >
bq. > I'd return an IllegalArgumentException instead. I believe it's an
error and not an option for the user to pass data which is null?
Agreed, good way of doing so, doing so.
bq. On 2012-02-16 03:26:37, Francis Liu wrote:
bq. > /trunk/src/java/org/apache/hcatalog/data/HCatRecordObjectInspector.java,
line 48
bq. > <https://reviews.apache.org/r/3819/diff/3/?file=75358#file75358line48>
bq. >
bq. > Nit: I would throw an IllegalArgumentException instead.
Done.
bq. On 2012-02-16 03:26:37, Francis Liu wrote:
bq. > /trunk/src/java/org/apache/hcatalog/data/HCatRecordSerDe.java, line 52
bq. > <https://reviews.apache.org/r/3819/diff/3/?file=75359#file75359line52>
bq. >
bq. > I see, this is no longer really a SerDe as hive would expect it to
be, it's more a converter. Just thinking if it'd be better for it not to
implement SerDe. Though we don't have to do that now, since this is already
checked in.
It's used as a convertor from this patch's code, but it is a SerDe that uses
HCatRecord as its writable type and as its object representation. I'd prefer to
retain it as-is rather than to strip it for utility methods in case we ever
revisit the need for a HCatRecordSerDe.
bq. On 2012-02-16 03:26:37, Francis Liu wrote:
bq. > /trunk/src/java/org/apache/hcatalog/data/LazyHCatRecord.java, line 34
bq. > <https://reviews.apache.org/r/3819/diff/3/?file=75361#file75361line34>
bq. >
bq. > Nit: why no just catch the specific exception. less noise in the
stack trace.
Since your previous issue was regarding the confusing nature of why we call a
serializeField call from within a deserialize method, and because that was set
up to throw an Exception instead of a SerDeException whose incomplete cleanup
lead to this, I'm now removing the deserialize method inside LazyHCatRecord
altogether and inlining a call to serializeField from within get, and having it
explicitly accept only SerDeException. Hopefully makes the code easier to read
as well.
bq. On 2012-02-16 03:26:37, Francis Liu wrote:
bq. > /trunk/src/java/org/apache/hcatalog/data/LazyHCatRecord.java, line 55
bq. > <https://reviews.apache.org/r/3819/diff/3/?file=75361#file75361line55>
bq. >
bq. > throw UnsupportedOperationException instead.
Ok, and done for the rest of them as well.
bq. On 2012-02-16 03:26:37, Francis Liu wrote:
bq. > /trunk/src/java/org/apache/hcatalog/data/LazyHCatRecord.java, line 35
bq. > <https://reviews.apache.org/r/3819/diff/3/?file=75361#file75361line35>
bq. >
bq. > Nit: IllegalStateException...
Changed.
- Sushanth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3819/#review5150
-----------------------------------------------------------
On 2012-02-15 23:13:50, Sushanth Sowmyan wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/3819/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-02-15 23:13:50)
bq.
bq.
bq. Review request for Alan Gates and Francis Liu.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Initial upload.
bq.
bq.
bq. This addresses bug HCATALOG-241.
bq. https://issues.apache.org/jira/browse/HCATALOG-241
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. /trunk/src/java/org/apache/hcatalog/common/HCatUtil.java 1244779
bq. /trunk/src/java/org/apache/hcatalog/data/DefaultHCatRecord.java 1244779
bq. /trunk/src/java/org/apache/hcatalog/data/HCatRecord.java 1244779
bq. /trunk/src/java/org/apache/hcatalog/data/HCatRecordObjectInspector.java
1244779
bq. /trunk/src/java/org/apache/hcatalog/data/HCatRecordSerDe.java 1244779
bq. /trunk/src/java/org/apache/hcatalog/data/HCatRecordable.java 1244779
bq. /trunk/src/java/org/apache/hcatalog/data/LazyHCatRecord.java
PRE-CREATION
bq. /trunk/src/test/org/apache/hcatalog/data/TestDefaultHCatRecord.java
1244779
bq. /trunk/src/test/org/apache/hcatalog/data/TestHCatRecordSerDe.java
1244779
bq.
/trunk/src/test/org/apache/hcatalog/mapreduce/TestHCatDynamicPartitioned.java
1244779
bq.
/trunk/src/test/org/apache/hcatalog/rcfile/TestRCFileInputStorageDriver.java
1244779
bq.
bq. Diff: https://reviews.apache.org/r/3819/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Sushanth
bq.
bq.
> Changes to HCatRecord to support switch from StorageDriver to SerDe
> -------------------------------------------------------------------
>
> Key: HCATALOG-241
> URL: https://issues.apache.org/jira/browse/HCATALOG-241
> Project: HCatalog
> Issue Type: Sub-task
> Affects Versions: 0.4
> Reporter: Alan Gates
> Assignee: Sushanth Sowmyan
> Fix For: 0.4
>
> Attachments: HCATALOG-241.patch, HCATALOG-241.patch.2,
> HCATALOG-241.patch.3
>
>
> This JIRA tracks changes to HCatRecord. See HCATALOG-237 for details and
> design notes.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
