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

[Managed BigQuery] use file loads with Avro format for better performance #33392

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

Conversation

ahmedabu98
Copy link
Contributor

No description provided.

@ahmedabu98 ahmedabu98 marked this pull request as ready for review December 16, 2024 18:02
Copy link
Contributor

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

@ahmedabu98
Copy link
Contributor Author

Failing test is not relevant

@ahmedabu98
Copy link
Contributor Author

assign set of reviewers

Copy link
Contributor

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @damondouglas for label java.
R: @shunping for label io.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

@chamikaramj
Copy link
Contributor

LGTM.

Can we add a unit test so that this doesn't regress ?

@ahmedabu98
Copy link
Contributor Author

Can we add a unit test so that this doesn't regress ?

Not sure what you mean by regress?

@chamikaramj
Copy link
Contributor

I mean to make sure we do not accidentally change the default from Avro back to Json.

@ahmedabu98
Copy link
Contributor Author

Is that necessary? We're just adjusting the write configuration here.

I'll add a comment to warn against changing this, would that be sufficient?

@ahmedabu98 ahmedabu98 added this to the 2.62.0 Release milestone Dec 18, 2024
@kennknowles
Copy link
Member

I think Cham's point is that someone else messing about in the code without knowing the context will not know what can or cannot be changed, and also might just make a mistake. So having a tiny test that makes sure they don't simply undo this change is the idea.

@ahmedabu98
Copy link
Contributor Author

If we're serious about that risk, we'd be doing this type of test all over the place 😅

I'll add a test anyways to get this PR in

@chamikaramj
Copy link
Contributor

Up to you.

Agree that we don't need such unit tests for all configs but might be useful to have some inexpensive unit tests to guard potentially expensive accidental regressions.

@ahmedabu98
Copy link
Contributor Author

Thanks, that's fair.
I added a short test that asserts the SchemaTransform uses Avro format. Will merge when tests go green

Copy link

codecov bot commented Dec 19, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.

Project coverage is 58.99%. Comparing base (d7502fa) to head (e04a3e5).
Report is 48 commits behind head on master.

Files with missing lines Patch % Lines
...gquery/providers/PortableBigQueryDestinations.java 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #33392      +/-   ##
============================================
+ Coverage     57.38%   58.99%   +1.60%     
- Complexity     1475     3184    +1709     
============================================
  Files           973     1139     +166     
  Lines        154978   175604   +20626     
  Branches       1076     3367    +2291     
============================================
+ Hits          88939   103591   +14652     
- Misses        63829    68658    +4829     
- Partials       2210     3355    +1145     
Flag Coverage Δ
java 70.34% <71.42%> (+1.76%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants