Skip to content

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

Open
sbrauer wants to merge 3 commits into
FundingCircle:masterfrom
sbrauer:add-record-name-metadata-to-deserialized-avro
Open

New feature: Add schema metadata to deserialized avro records#174
sbrauer wants to merge 3 commits into
FundingCircle:masterfrom
sbrauer:add-record-name-metadata-to-deserialized-avro

Conversation

@sbrauer

@sbrauer sbrauer commented Jul 26, 2019

Copy link
Copy Markdown
Contributor

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 <sam.brauer@gmail.com>
@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
Comment thread src/jackdaw/serdes/avro.clj Outdated
[field-key (avro->clj field-coercion value)]))))
(vals field->schema+coercion))
{:name (.getName schema)
:fullname (.getFullName schema)})))

Copy link
Copy Markdown
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
Copy Markdown
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.

Signed-off-by: Sam Brauer <sam.brauer@gmail.com>
@codecov

codecov Bot commented Jul 27, 2019

Copy link
Copy Markdown

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

codecov Bot commented Jul 27, 2019

Copy link
Copy Markdown

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

sbrauer commented Jul 30, 2019

Copy link
Copy Markdown
Contributor Author

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

cddr commented Aug 1, 2019

Copy link
Copy Markdown
Contributor

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 <sam.brauer@gmail.com>
@sbrauer

sbrauer commented Aug 1, 2019

Copy link
Copy Markdown
Contributor Author

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
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 64.70588% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.89%. Comparing base (775435c) to head (2cdba4f).
⚠️ Report is 354 commits behind head on master.

Files with missing lines Patch % Lines
src/jackdaw/serdes/avro.clj 64.70% 5 Missing and 1 partial ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #174      +/-   ##
==========================================
- Coverage   80.92%   80.89%   -0.03%     
==========================================
  Files          39       39              
  Lines        2364     2366       +2     
  Branches      149      149              
==========================================
+ Hits         1913     1914       +1     
- Misses        302      303       +1     
  Partials      149      149              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

3 participants