Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New feature: Add schema metadata to deserialized avro records #174

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sbrauer
Copy link
Contributor

@sbrauer sbrauer commented Jul 26, 2019

When an Avro record is deserialized to a Clojure map, add the record's name and fully-qualified name to the map as metadata.
These record names are potentially useful info that was being discarded.

When an Avro record is deserialized to a Clojure map, add the record's schema to the map as metadata.

My personal use case for this metadata is to make it easier for consuming code to deal with unions of records.

When an Avro record is deserialized to a Clojure map, add the record's
name and fully-qualified name to the map as metadata.
These record names are potentially useful info that was being discarded.

My personal use case for this metadata is to make it easier for
consuming code to deal with unions of records.

Signed-off-by: Sam Brauer <[email protected]>
@sbrauer sbrauer requested a review from a team as a code owner July 26, 2019 20:53
@sbrauer sbrauer changed the title New feaure: Add record name metadata to deserialized avro records New feature: Add record name metadata to deserialized avro records Jul 26, 2019
[field-key (avro->clj field-coercion value)]))))
(vals field->schema+coercion))
{:name (.getName schema)
:fullname (.getFullName schema)})))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think when hyphenating the FullName symbol I'd expect to see full-name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just pushed a commit with the hyphenated name.

@codecov
Copy link

codecov bot commented Jul 27, 2019

Codecov Report

Merging #174 into master will decrease coverage by 0.1%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #174      +/-   ##
==========================================
- Coverage   80.92%   80.81%   -0.11%     
==========================================
  Files          39       39              
  Lines        2364     2367       +3     
  Branches      149      149              
==========================================
  Hits         1913     1913              
- Misses        302      305       +3     
  Partials      149      149
Impacted Files Coverage Δ
src/jackdaw/serdes/avro.clj 88% <66.66%> (+0.11%) ⬆️
src/jackdaw/serdes/edn.clj 80.76% <0%> (-11.54%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 775435c...e3f45eb. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 27, 2019

Codecov Report

Merging #174 into master will increase coverage by 0.02%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #174      +/-   ##
==========================================
+ Coverage   80.92%   80.94%   +0.02%     
==========================================
  Files          39       39              
  Lines        2364     2367       +3     
  Branches      149      149              
==========================================
+ Hits         1913     1916       +3     
  Misses        302      302              
  Partials      149      149
Impacted Files Coverage Δ
src/jackdaw/serdes/avro.clj 88% <66.66%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 775435c...2cdba4f. Read the comment docs.

@sbrauer
Copy link
Contributor Author

sbrauer commented Jul 30, 2019

I hope you agree that there's value in exposing the record names. There may be other ways to do that besides metadata (although it was the best way I could think of), and even with the metadata approach one might have chosen different keywords.
I'd be happy to discuss.

@cddr
Copy link
Contributor

cddr commented Aug 1, 2019

Hey Sam. Sorry for the radio silence so far. The other person in our company who might have an opinion on this is away on holiday at the moment.

I agree that this is valuable information that should be made available and that given the current setup, the metadata seems like the only place to put it. I wonder whether we could just put the whole schema in the metadata (or maybe even both a writer schema and a reader schema).

Adding the record names only was probably too limiting and opinionated.
By exposing the record schema object a consumer has more flexibility
to get the names and lots of other data besides.

Signed-off-by: Sam Brauer <[email protected]>
@sbrauer
Copy link
Contributor Author

sbrauer commented Aug 1, 2019

No worries. Good to hear that you're open to the general idea. I've also been thinking that exposing the schema itself could be better than exposing the names (since the names and a lot of other info can be obtained from the schema; on the other hand one could argue it's exposing too much internal detail).

I must admit that I haven't yet taken the time to digest your recent change related to decoupling the reader and writer schemas. I should do that soon. In the meantime I pushed a commit that adds the schema to the metadata instead of the names. I'll update the PR title as well.

@sbrauer sbrauer changed the title New feature: Add record name metadata to deserialized avro records New feature: Add schema metadata to deserialized avro records Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants