Implements Feature 630 #634
Conversation
|
Thanks for the contribution! Please review the labels and make any necessary changes. |
Hey miss bot, where did the rusted robot go? |
|
|
Thanks for the contribution! Please review the labels and make any necessary changes. |
Codecov Report
@@ Coverage Diff @@
## master #634 +/- ##
=======================================
- Coverage 78% 78% -1%
=======================================
Files 310 311 +1
Lines 16896 17181 +285
=======================================
+ Hits 13330 13451 +121
- Misses 3566 3730 +164
Continue to review full report at Codecov.
|
rust-toolchain
Outdated
| @@ -1 +1 @@ | |||
| 1.52.1 | |||
| nightly-2021-05-06 | |||
There was a problem hiding this comment.
We have some compile error that are not resolved in nightly-2021-05-06.
thread 'rustc' panicked at 'assertion failed: `(left == right)`
left: `Some(Fingerprint(8537439170242672706, 4648092694241280842))`,
right: `Some(Fingerprint(212923312148573672, 18320173992410164289))`: found unstable fingerprints for evaluate_obligation(6b017a932d0761bd-4ff2d1ea8919cfd7): Ok(EvaluatedToOk)', /rustc/bacf770f2983a52f31e3537db5f0fe1ef2eaa874/compiler/rustc_query_system/src/query/plumbing.rs:585:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: internal compiler error: unexpected panic
note: the compiler unexpectedly panicked. this is a bug.
note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md
note: rustc 1.54.0-nightly (bacf770f2 2021-05-05) running on x86_64-apple-darwin
note: compiler flags: -C embed-bitcode=no -C split-debuginfo=unpacked -C debuginfo=2 -C incremental
note: some of the compiler flags provided by cargo are hidden
query stack during panic:
#0 [evaluate_obligation] evaluating trait selection obligation `common_datavalues::DataValue: std::marker::Send`
#1 [typeck] type-checking `tests::service::start_one_service`
end of query stack
There was a problem hiding this comment.
yeah, I have the same problem in my dev env, especially when using the cargo watch xxx tool, the error you mentioned keeps popping out.
Since that, I switch to stable toolchain 1.52.1 during daily dev.
But I am not sure if that's appropriate (in case somebody or tools is using nightly features). So I switch it back in the last commit.
Any suggestions?
There was a problem hiding this comment.
@BohuTANG I've seen a gist that you created that keeps a similar error message, any idea about it?
There was a problem hiding this comment.
Sorry for the later, grcov nightly Rust is required:
https://github.com/mozilla/grcov
But we can try to rollback to a stable nightly version.
There was a problem hiding this comment.
Got it, nightly is ok.
There was a problem hiding this comment.
FYI, the master rust toolchain has changed to nightly-2021-06-01, seems more stable.
dantengsky
force-pushed
the
feature-630
branch
2 times, most recently
from
d04cf38
to
a4a96b4
Compare
|
@dantengsky ready for review? I received a review request but it is still a draft |
| pub async fn scan_partition( | ||
| &mut self, | ||
| db_name: String, | ||
| tbl_name: String, | ||
| scan_plan: &ScanPlan, |
There was a problem hiding this comment.
What about scan or scan_table?
The subject is a table, not a partition.
BTW on the server-side, the corresponding function name has an extra s: scan_partitions
There was a problem hiding this comment.
change method name "scan_partition" to "scan", and "read_partition" to "scan_table"?
Oh, wait, u reviewed last night? thanks for that bro. I've force pushed something for the "final touch", including changing the name of method "read" to "read_partition". any suggestion about the now names?
There was a problem hiding this comment.
what about scan_table_partitions and read_table_partition? since the former filters table partitions, the latter returns datablocks of a partition.
There was a problem hiding this comment.
what about
scan_table_partitionsandread_table_partition? since the former filters table partitions, the latter returns datablocks of a partition.
Nice to me.
@BohuTANG @sundy-li Is there any difference between a datablock and a partition in our code base?
If no, using a universal term would be better for stargazers.
There was a problem hiding this comment.
Oh, wait, u reviewed last night? thanks for that bro. I've force pushed something for the "final touch"
Not that late for me :DDD
common/flights/src/store_do_get.rs
Outdated
| pub push_down: PlanNode, | ||
| } | ||
|
|
||
| #[derive(serde::Serialize, serde::Deserialize, Clone, Debug)] | ||
| pub struct ScanPartitionsAction { | ||
| pub can_plan: ScanPlan, |
There was a problem hiding this comment.
| pub can_plan: ScanPlan, | |
| pub scan_plan: ScanPlan, |
| if let Err(_e) = tx | ||
| .send(flight_data_from_arrow_batch(&batch, &ipc_write_opt).1) | ||
| .await | ||
| { |
There was a problem hiding this comment.
yeah, log::error is better
It was too late last night, so I invited you but keep this PR in the draft state. But.... seems I'd rather invite you later ;-) |
|
CI Passed |
|
CI Passed |
1 similar comment
|
CI Passed |
|
CI Passed |
5 similar comments
|
CI Passed |
|
CI Passed |
|
CI Passed |
|
CI Passed |
|
CI Passed |
|
CI Passed |
18 similar comments
|
CI Passed |
|
CI Passed |
|
CI Passed |
|
CI Passed |
|
CI Passed |
|
CI Passed |
|
CI Passed |
|
CI Passed |
|
CI Passed |
|
CI Passed |
|
CI Passed |
|
CI Passed |
|
CI Passed |
|
CI Passed |
|
CI Passed |
|
CI Passed |
|
CI Passed |
|
CI Passed |
Summary
It's a baby step of integrating Store with Query, which implements
Basic statements like
insert into ...andselect .. from ...could be executed now. (and lots of interesting things are left to do)Changelog
Store: implementions for ITable read_plan and read a5c42b2
Query: implements RemoteTalbe's read_plan & read b55eacf
Adds remote flag to ReadDataSourcePlan deaea8e
Tweaks stateless test cases ed69c92
The following issues might be worthy of your concern:
Remove trait bound Sync from SendableDataBlockStream ed69c92
Turns out, at least for now, we do not need this trait bound, and without
Syncconstraint, SendableDataBlockStream is more stream-combinator friendly.Keep
ITable::read_planas a non-async methodBy using runtime of ctx (and channel).
IMHO, change
ITable::read_planto async fn may be too harsh at this stage.Add an extra flag to
ReadDataSourcePlanandSourceTransformSo that we could be aware of operating a remote table(and fetch remote table accordingly). It is a temp workaround, let's postpone it until the
CatalogAPI is ready.SourceTransform::executeandFuseQueryContextare tweaked accordingly. pls see deaea8eRelated Issues
resolves #630
Test Plan
Progress