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

fix: Put null helpers back #1002

Merged
merged 6 commits into from
Jun 29, 2023
Merged

fix: Put null helpers back #1002

merged 6 commits into from
Jun 29, 2023

Conversation

candiduslynx
Copy link
Contributor

Return of the code from #913

@github-actions github-actions bot added the fix label Jun 27, 2023
@codecov
Copy link

codecov bot commented Jun 27, 2023 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (main@466796b). Click here to learn what that means.
Patch coverage: 0.00% of modified lines in pull request are covered.

❗ Current head f5f6209 differs from pull request most recent head 68f8557. Consider uploading reports for the commit 68f8557 to get more accurate results

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1002   +/-   ##
=======================================
  Coverage        ?   47.94%           
=======================================
  Files           ?       77           
  Lines           ?     7463           
  Branches        ?        0           
=======================================
  Hits            ?     3578           
  Misses          ?     3553           
  Partials        ?      332
Impacted Files Coverage Δ
plugin/nulls.go 0.00% <0.00%> (ø)
plugin/testing_write.go 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link

github-actions bot commented Jun 27, 2023 •
edited
Loading

⏱️ Benchmark results

  • Glob-2 ns/op: 246.8

hermanschaaf
hermanschaaf reviewed Jun 27, 2023
View reviewed changes
plugin/nulls.go
)

//nolint:unused
func stripNullsFromLists(records []arrow.Record) {
Copy link
Member

Choose a reason for hiding this comment

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

Why are we adding unused code? Will there be a follow-up where it gets used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I was just migrating CH when I noticed this missing.

I can comment out the relevant section in CH tests, but it will still be required once we bring the vast testing back

Copy link
Member

Choose a reason for hiding this comment

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

We can leave this here as a reminder, but can we rather add it back when it's needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💯

hermanschaaf
hermanschaaf reviewed Jun 27, 2023
View reviewed changes
plugin/nulls.go Outdated Show resolved Hide resolved
candiduslynx and others added 3 commits June 29, 2023 21:40
@candiduslynx @hermanschaaf
Update plugin/nulls.go
798e1ef 
Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
@candiduslynx
upd todo
3108343
@candiduslynx
lint
f5f6209
yevgenypats
yevgenypats approved these changes Jun 29, 2023
View reviewed changes
Copy link
Member

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

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

Im approving to unblock but in general I think we should somehow come with an approach that is either on the plugin side or generalize better otherwise the testing code is going to get out of hand quickly.

@kodiakhq kodiakhq bot merged commit 95ed5df into main Jun 29, 2023
6 checks passed
@kodiakhq kodiakhq bot deleted the feat/allow-null-test-opt branch June 29, 2023 19:47
@cq-bot cq-bot mentioned this pull request Jun 29, 2023
Merged
kodiakhq bot pushed a commit that referenced this pull request Jun 29, 2023
@cq-bot
chore(main): Release v4.3.1-rc1 (#1024)
2df14a8 
🤖 I have created a release *beep* *boop*
---


## [4.3.1-rc1](v4.3.0-rc1...v4.3.1-rc1) (2023-06-29)


### Bug Fixes

* Enable double migration test ([#1023](#1023)) ([466796b](466796b))
* Put null helpers back ([#1002](#1002)) ([95ed5df](95ed5df))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
@candiduslynx candiduslynx mentioned this pull request Jul 5, 2023
Merged
kodiakhq bot pushed a commit that referenced this pull request Jul 5, 2023
@candiduslynx
fix: Handle null-related test options (#1074)
88f08ee 
Follow-up for #1072 & #1002
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hermanschaaf hermanschaaf hermanschaaf left review comments

@yevgenypats yevgenypats yevgenypats approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@candiduslynx @hermanschaaf @yevgenypats