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

[deps] bump arrow2 to d14ae86 #3868

Merged
merged 4 commits into from
Jan 24, 2022
Merged

Conversation

PsiACE
Copy link
Member

@PsiACE PsiACE commented Jan 16, 2022

Signed-off-by: Chojan Shang [email protected]

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

  • d14ae86
  • fix stateless tests

TODO

  • 9b54146
  • ?

Changelog

  • Improvement
  • Build/Testing/CI
  • Not for changelog (changelog entry is not required)

Related Issues

Related #3746

Test Plan

Unit Tests

Stateless Tests

Signed-off-by: Chojan Shang <[email protected]>
@PsiACE PsiACE requested a review from sundy-li January 16, 2022 13:56
@PsiACE PsiACE requested a review from BohuTANG as a code owner January 16, 2022 13:56
@databend-bot databend-bot added pr-build this PR changes build/testing/ci steps pr-improvement labels Jan 16, 2022
@databend-bot
Copy link
Member

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

2 similar comments
@databend-bot
Copy link
Member

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

@databend-bot
Copy link
Member

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

@vercel
Copy link

vercel bot commented Jan 16, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

databend – ./website

🔍 Inspect: https://vercel.com/databend/databend/8eTCpRPjTcBEjvsUuspWaHFiq9EF
✅ Preview: https://databend-git-fork-psiace-bump-arrow2-databend.vercel.app

Signed-off-by: Chojan Shang <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Jan 16, 2022

Codecov Report

Merging #3868 (6e75e08) into main (dc8bfed) will decrease coverage by 0%.
The diff coverage is 47%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3868   +/-   ##
=====================================
- Coverage     57%     57%   -1%     
=====================================
  Files        800     800           
  Lines      42853   42864   +11     
=====================================
+ Hits       24429   24434    +5     
- Misses     18424   18430    +6     
Impacted Files Coverage Δ
query/src/api/rpc/flight_client_stream.rs 0% <0%> (ø)
query/src/api/rpc/flight_service_stream.rs 19% <50%> (+3%) ⬆️
query/src/api/rpc/flight_dispatcher.rs 84% <100%> (ø)
query/src/api/rpc/flight_service.rs 54% <100%> (+1%) ⬆️
common/containers/src/pool.rs 77% <0%> (-3%) ⬇️
common/datavalues/src/columns/data_column.rs 71% <0%> (+<1%) ⬆️
metasrv/src/meta_service/meta_service_impl.rs 93% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc8bfed...6e75e08. Read the comment docs.

@sundy-li
Copy link
Member

--- /Users/runner/work/databend/databend/tests/suites/0_stateless/02_0045_function_in.result	2022-01-16 15:57:58.000000000 +0000
+++ /Users/runner/work/databend/databend/tests/suites/0_stateless/02_0045_function_in.stdout	2022-01-16 16:01:17.000000000 +0000
@@ -1,17 +1,3 @@
-1
-2
-1
-2
-19
-4999
-0
-3
-4
-5
-6
-7
-8
-9
 NULL
 1
 0

Flaky test of function in, cc @Veeupup

@PsiACE
Copy link
Member Author

PsiACE commented Jan 17, 2022

Flaky test of function in

Other tests also fail, 😢 :

@PsiACE PsiACE marked this pull request as draft January 17, 2022 02:27
@sundy-li
Copy link
Member

sundy-li commented Jan 18, 2022

Some tests are failed.

@jorgecarleitao Do you know what's the big change in flight if we bump arrow2 into d14ae86
(jorgecarleitao/arrow2#713) ?

Errors:

{"v":0,"name":"[email protected]:3307","msg":"panicked at 'called `Result::unwrap()` on an `Err` value: OutOfSpec(\"Unable to get head as schema\")', query/src/api/rpc/flight_client_stream.rs:56:79","level":50,"hostname":"arch","pid":31036,"time":"2022-01-17T09:33:22.311028756Z","target":"common_tracing::panic_hook","line":24,"file":"common/tracing/src/panic_hook.rs","panic.column":79,"panic.line":56,"panic.file":"query/src/api/rpc/flight_client_stream.rs"}

@jorgecarleitao
Copy link

jorgecarleitao commented Jan 18, 2022

@jorgecarleitao Do you know what's the big change in flight if we bump arrow2 into d14ae86 (jorgecarleitao/arrow2#713) ?

Thus far, Field had a dict_id. However, dict_id is IPC-specific. To reuse dictionaries over the IPC, users need to be careful in writing the dict_id. The old API did not allow this very well, as dict_id was just set to some default value.
For this reason, I have separated the dict_id from Field into its own little struct (IPCField), so that we can think about arrow's logical Field without having to think about how it is represented in IPC, and also allow changing it without having to modify Field, as it usually indicates a change in the logical construct.

* try get schema from stream_info

Signed-off-by: Chojan Shang <[email protected]>

* fix: read batch messages

Signed-off-by: Chojan Shang <[email protected]>
@PsiACE
Copy link
Member Author

PsiACE commented Jan 24, 2022

PTAL @sundy-li , the rest will be done in other PRs

@sundy-li sundy-li merged commit 60bba3d into databendlabs:main Jan 24, 2022
@PsiACE PsiACE deleted the bump-arrow2 branch January 24, 2022 04:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need-review pr-build this PR changes build/testing/ci steps
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants