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.

Sign up for GitHub

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

Jump to bottom

Breaking: Reduce number of google enums. Introduce zetasql for query verification and analysis #121

Merged
merged 10 commits into from
Aug 30, 2023
Merged

Breaking: Reduce number of google enums. Introduce zetasql for query verification and analysis #121

merged 10 commits into from
Aug 30, 2023

Conversation

hamnis
Copy link
Contributor

@hamnis hamnis commented Jul 13, 2023 •
edited
Loading

Removes all uses of StandardSQLTypeName and Field.Mode from our types
Replaces with BQField.Type and BQField.Mode

We should have a test which tests all cases are represented in our types
Removes internal google types from PartitionType
Added conversion from / to google types in PartitionTypeHelper and SchemaHelper

Depends on #127

@hamnis hamnis force-pushed the extract/own-types branch 5 times, most recently from 0246156 to 04af07f Compare July 28, 2023 06:23
@hamnis hamnis marked this pull request as ready for review August 22, 2023 10:07
@hamnis hamnis changed the title Extract/own types Breaking: Reduce number of google enums. Introduce zetasql for query verification and analysis Aug 24, 2023
@ingarabr
Copy link
Contributor

Great work! The usage of StandardSQLTypeName and Field.Mode has always been an annoyance for me. Getting everything under one package/namespace will improve the usability of the library.

I like the idea of using zetasql but it has one caveats:
It does not have native support for mac m1. You can get it to work using a x86 JVM. Not sure how that impacts performance. I'm not sure about windows but I assume it works fine with WSL. Due to those limitations, maybe we should have the option to fallback to calling BigQuery?

@hamnis
Copy link
Contributor Author

hamnis commented Aug 24, 2023 •
edited
Loading

@ingarabr we were able to run the tests on an m1 mac without rosetta, so that would be an indication of success.
This is regardless a separate module, so it is opt-in.

The tests were run against the wrong branch, so I have disabled the tests for aarch64

@hamnis hamnis force-pushed the extract/own-types branch 4 times, most recently from 38c930a to 06700f9 Compare August 25, 2023 07:53
HenningKoller
HenningKoller approved these changes Aug 28, 2023
View reviewed changes
Copy link
Contributor

@HenningKoller HenningKoller left a comment

Choose a reason for hiding this comment

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

Nice! 👍

@hamnis
Remove use of google types
0384082 
* Introduce new internal helper objects
* Remove uses of Field.Mode and StandardSQLTypeName
* Add conversion tests
@hamnis
We can use offline zeta sql to verify queries
bff0374 
* Analyze the query before rendering it to extract referenced tables
* Bump to jdk11
@hamnis
use tagless final
2f9aa0b
@hamnis
Disable tests for aarch64
abaf7ea
@hamnis
Fixes after rebase
1997c2f
@hamnis
Bump series
46ae8fd
ingarabr
ingarabr reviewed Aug 30, 2023
View reviewed changes
core/src/main/scala/no/nrk/bigquery/BQPartitionType.scala
.setField(field.value)
.build()
)
final case class DatePartitioned(field: Ident) extends BQPartitionType[LocalDate]
Copy link
Contributor

Choose a reason for hiding this comment

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

It had been convenient to have BQField here if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lets pick that up in a follow-up

ingarabr
ingarabr approved these changes Aug 30, 2023
View reviewed changes
Copy link
Contributor

@ingarabr ingarabr left a comment

Choose a reason for hiding this comment

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

Looking good!

@hamnis hamnis merged commit dbdde6b into main Aug 30, 2023
11 checks passed
@hamnis hamnis deleted the extract/own-types branch August 30, 2023 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HenningKoller HenningKoller HenningKoller approved these changes

@ingarabr ingarabr ingarabr approved these changes

Assignees
No one assigned
Labels
breaking
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hamnis @ingarabr @HenningKoller