Skip to content
This repository was archived by the owner on Jun 15, 2021. It is now read-only.

Conversation

@guilload
Copy link

@guilload guilload commented Jun 20, 2020

As previously discussed, here is my PR implementing custom object inspectors for the IcebergSerDe. As a consequence, the class is greatly simplified. I also slightly refactored the TableResolver to write some tests without having to set the table name.

I was able to read a table written with Spark with this SerDe so I have some confidence in the implementation. However, I maybe have missed some subtleties when converting Iceberg objects to Hive objects, mostly for the decimal and timestamp types.

🥖

@guilload guilload force-pushed the guilload--implement-object-inspectors branch from 0b4d6ef to 8bbcf7b Compare June 20, 2020 03:39
@massdosage massdosage requested a review from teabot June 22, 2020 09:52
@cmathiesen
Copy link

Thanks for the PR @guilload, we're working on a review and testing this out in a branch with the Hiveberg project :D

@guilload guilload force-pushed the guilload--implement-object-inspectors branch from 2de7589 to 3b18a5a Compare June 25, 2020 21:19
@massdosage massdosage merged commit f7c5c39 into iceberg-serde Jun 26, 2020
@massdosage massdosage deleted the guilload--implement-object-inspectors branch June 26, 2020 11:16
return null;
default:
throw new NoSuchTableException("Table does not exist at location: " + tableLocation);
throw new RuntimeException("Catalog " + catalogName + " not supported.");
Copy link

Choose a reason for hiding this comment

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

nit: throw NoSuchNamespacException

Choose a reason for hiding this comment

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

Sure, we can address that in https://github.com/apache/iceberg/pull/1103/files#diff-64ac74e3513bf6536c0a39f377c48ce5R62 (I'll make the change now, also spotted that the tests for this class could be fleshed out and tidied up a bit)

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.

5 participants