[
https://issues.apache.org/jira/browse/HCATALOG-241?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13204725#comment-13204725
]
[email protected] commented on HCATALOG-241:
--------------------------------------------------------
bq. On 2012-02-09 01:56:49, Alan Gates wrote:
bq. > /trunk/src/java/org/apache/hcatalog/data/HCatRecordSerDe.java, line 197
bq. > <https://reviews.apache.org/r/3819/diff/1/?file=73307#file73307line197>
bq. >
bq. > Why did this move from private to public static? Seems like it's
only used by LazyHCatRecord, so it could at least be package private.
bq.
bq. Sushanth Sowmyan wrote:
bq. I changed it to static because I saw no reason for it to be an object
method, and changed it to public because I felt it could be used as a utility
function - I had initially wrapped it and called it from HCatUtil before
changing that to just go from LazyHCatRecord itself. Now, there's no reason for
it, and I can change it back.
Just my 2 cents. Silent errors are bad, mysql has them and adds a lot of
overhead while debugging. This failure is obviously not recoverable since your
gonna give an inconsistent view of the row. So why not throw
IllegalStateException instead?
bq. On 2012-02-09 01:56:49, Alan Gates wrote:
bq. > /trunk/src/java/org/apache/hcatalog/data/LazyHCatRecord.java, line 33
bq. > <https://reviews.apache.org/r/3819/diff/1/?file=73309#file73309line33>
bq. >
bq. > In the design notes posted on HCATALOG-237 we had discussed not
caching deserialized values. Doing so increases your memory usage for likely
no reason (usually records are only read once).
bq.
bq. Sushanth Sowmyan wrote:
bq. True, and I did start out that way, but LazyHCatRecord extends
HCatRecord which implements WritableComparable - this means the following:
bq.
bq. a) List<Object> .getAll() is something we need to support - and this
is something that can be called by layers above us, which means we do need a
fully deserialized version if requested anyway.
bq. b) For .compareTo() to work, same deal.
bq.
bq. I can take it out and make it only locally deserialize the entire
object when needed.
I remember us talking about getAll() in one of the meetings, I believe we were
leaning towards removing it. Is using compareTo and getAll() a common use case?
I believe most users would like to compare certain fields? Not the entire
record. As for equality most would just compare the primary keys? For the
outputSide I would need getAll() to pass the contents to SerDe but I could
explicity call each one by one or we could have a the other implementation of
HCatRecord (ie MutableHCatRecord) to have getAll()?
bq. On 2012-02-09 01:56:49, Alan Gates wrote:
bq. > /trunk/src/java/org/apache/hcatalog/data/HCatRecordObjectInspector.java,
line 51
bq. > <https://reviews.apache.org/r/3819/diff/1/?file=73306#file73306line51>
bq. >
bq. > This is a significant behavioral change, to now return null rather
than crash when we can't decode a field. It may be ok, but are we sure? Also,
the log message here should be at least a warn, not a debug which will usually
get ignored.
bq.
bq. Sushanth Sowmyan wrote:
bq. This one is something I had misgivings about, and I'm still a bit
on-the-edge.
bq.
bq. The method we override does not allow throwing an exception, so we
aren't allowed to throw an exception. Earlier, the get method we depended on
wouldn't/couldn't throw an exception. Earlier, we had the objects at hand, and
were only retrieving it. Now, the underlying get can.
bq.
bq. The case where it manifests itself is if the underlying object has
malformed data that causes a deserialization error, and we choose to ignore
that and present nulls to the layers above. I'm not sure I like the hiding
behaviour myself.
Just my 2 cents. Silent errors are bad, mysql has them and adds a lot of
overhead while debugging. This failure is obviously not recoverable since your
gonna give an inconsistent view of the row. So why not throw
IllegalStateException instead?
- Francis
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3819/#review4951
-----------------------------------------------------------
On 2012-02-09 00:54:29, 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-09 00:54:29)
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 1242037
bq. /trunk/src/java/org/apache/hcatalog/data/DefaultHCatRecord.java 1242037
bq. /trunk/src/java/org/apache/hcatalog/data/HCatRecord.java 1242037
bq. /trunk/src/java/org/apache/hcatalog/data/HCatRecordObjectInspector.java
1242037
bq. /trunk/src/java/org/apache/hcatalog/data/HCatRecordSerDe.java 1242037
bq. /trunk/src/java/org/apache/hcatalog/data/HCatRecordable.java 1242037
bq. /trunk/src/java/org/apache/hcatalog/data/LazyHCatRecord.java
PRE-CREATION
bq. /trunk/src/test/org/apache/hcatalog/data/TestHCatRecordSerDe.java
1242037
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
>
>
> 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
