Skip to content

fix: Put null helpers back#1002

Merged
kodiakhq[bot] merged 6 commits intomainfrom
feat/allow-null-test-opt
Jun 29, 2023
Merged

fix: Put null helpers back#1002
kodiakhq[bot] merged 6 commits intomainfrom
feat/allow-null-test-opt

Conversation

@candiduslynx
Copy link
Copy Markdown
Contributor

@candiduslynx candiduslynx commented Jun 27, 2023

Return of the code from #913

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

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

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
Copy Markdown
Contributor

@hermanschaaf hermanschaaf Jun 27, 2023

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
Copy Markdown
Contributor Author

@candiduslynx candiduslynx Jun 27, 2023

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
Copy Markdown
Contributor

@hermanschaaf hermanschaaf Jun 27, 2023

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
Copy Markdown
Contributor Author

@candiduslynx candiduslynx Jun 27, 2023

Choose a reason for hiding this comment

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

💯

@candiduslynx candiduslynx force-pushed the feat/allow-null-test-opt branch from 9961903 to 08edb0f Compare June 27, 2023 09:49
hermanschaaf
hermanschaaf reviewed Jun 27, 2023
View reviewed changes
@candiduslynx candiduslynx force-pushed the feat/allow-null-test-opt branch from 08edb0f to 481df65 Compare June 29, 2023 11:10
@candiduslynx candiduslynx force-pushed the feat/allow-null-test-opt branch from 481df65 to 4fa4143 Compare June 29, 2023 18:29
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
Copy Markdown
Contributor

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

2 more reviewers

@hermanschaaf hermanschaaf hermanschaaf left review comments

@yevgenypats yevgenypats yevgenypats approved these changes

Reviewers whose approvals may not affect merge requirements

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