Skip to content

fix: Disallow null character in strings per utf8 spec - #797

Merged
kodiakhq[bot] merged 1 commit into
mainfrom
fix/utf8
Apr 19, 2023
Merged

fix: Disallow null character in strings per utf8 spec#797
kodiakhq[bot] merged 1 commit into
mainfrom
fix/utf8

Conversation

@yevgenypats

@yevgenypatsyevgenypats commented Apr 18, 2023

Copy link
Copy Markdown
Contributor

This should solve bunch of issues that we have with NullValues and StripNulls logic propagated to all our destination plugins. Apparently Valid utf8 string can't have null values in the middle of the string so we should just strip those in our type system (it can have at the end or the start but I suggest actually stripping those down as well as it's really unnecessary as far as I know and it exist prob for some archaic reason like sending a string over the wire without any additional encoding/protocol, which we have via arrow).

Once we move sources to arrow we will add the validation/strip on the source side.

@github-actions

Copy link
Copy Markdown

⏱️ Benchmark results

Comparing with b1baab6

  • DefaultConcurrencyDFS-2 resources/s: 11,261 ⬆️ 0.85% increase vs. b1baab6
  • DefaultConcurrencyRoundRobin-2 resources/s: 12,399 ⬇️ 0.51% decrease vs. b1baab6
  • Glob-2 ns/op: 193.6 ⬇️ 0.57% decrease vs. b1baab6
  • TablesWithChildrenDFS-2 resources/s: 28,259 ⬆️ 6.90% increase vs. b1baab6
  • TablesWithChildrenRoundRobin-2 resources/s: 25,461 ⬇️ 0.78% decrease vs. b1baab6
  • TablesWithRateLimitingDFS-2 resources/s: 28.25 ⬇️ 1.17% decrease vs. b1baab6
  • TablesWithRateLimitingRoundRobin-2 resources/s: 834.3 ⬆️ 0.50% increase vs. b1baab6
  • BufferedScanner-2 ns/op: 9.399 ⬆️ 0.09% increase vs. b1baab6
  • LogReader-2 ns/op: 30.71 (no change)

@codecov

codecovBot commented Apr 18, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage has no change and project coverage change: -0.02⚠️

Comparison is base (b1baab6) 46.73% compared to head (8fdddb7) 46.71%.

Additional details and impacted files
@@ Coverage Diff @@## main #797 +/- ##
==========================================
- Coverage 46.73% 46.71% -0.02% 
==========================================
Files 76 76 Lines 7830 7832 +2 ==========================================
Hits 3659 3659 - Misses 3680 3682 +2 
Partials 491 491 
Impacted FilesCoverage Δ
schema/arrow.go0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@kodiakhq
kodiakhqBot merged commit 591502f into mainApr 19, 2023
@kodiakhq
kodiakhqBot deleted the fix/utf8 branch April 19, 2023 07:50
@cq-botcq-bot mentioned this pull request Apr 19, 2023
kodiakhqBot pushed a commit that referenced this pull request Apr 19, 2023
🤖 I have created a release *beep* *boop*
---
## [2.3.2](v2.3.1...v2.3.2) (2023-04-19)
### Bug Fixes
* Arrow Retain and Release fixes ([#795](#795)) ([a893db6](a893db6))
* Disallow null character in strings per utf8 spec ([#797](#797)) ([591502f](591502f))
---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@yevgenypats@erezrokah