Feat/sql pagination 2 #157
Conversation
|
Marked as draft as I have not yet added testing for pagination |
tests/tui_cases/pagination.rs
Outdated
There was a problem hiding this comment.
the tests here dont quite test the entire flow like actually happens in the tui. it would be better if we could get a full end to end test with both a query being into to the editor and reading from a file with a known batch number so we could deterministically test
| let _ = _event_tx.send(AppEvent::FlightSQLQueryResult(query)); | ||
| }); | ||
| } | ||
| // KeyCode::Enter => { |
There was a problem hiding this comment.
Going to come back to this in a follow on PR for paginating FlightSQL
There was a problem hiding this comment.
I wonder if we can somehow hide it behind an interface (so the pagination code for FlightSQL and local are the same) 🤔
There was a problem hiding this comment.
I agree that would be nice. I want to finalize the functionality for flightsql first - then with both features complete i think it will be easier to reason about what the appropriate interface should be.
| } | ||
| }); | ||
| } | ||
| // #[cfg(feature = "flightsql")] |
There was a problem hiding this comment.
Coming back to this in follow on PR as well
|
@alamb I plan to merge this in the coming day or two (will leave for a bit in case your interested, but no pressure to review if you dont have time) and then work on flightsql pagination next. I had to comment out some stuff for flightsql that was dependent on the sql code - that will be fixed in the follow on pr. |
| pub fn record_batches_to_table<'frame, 'results>( | ||
| record_batches: &'results [RecordBatch], | ||
| record_batches: &'results [&RecordBatch], |
There was a problem hiding this comment.
not sure what the signature of this function will be yet - should be finalized in the flightsql PR
| @@ -14,78 +14,15 @@ | |||
| // KIND, either express or implied. See the License for the | |||
| // specific language governing permissions and limitations | |||
| // under the License. | |||
| // | |||
There was a problem hiding this comment.
| // |
There was a problem hiding this comment.
I briefly skimmed this PR @matthewmturner -- and it looks quite cool. Thank you 🙏
| let _ = _event_tx.send(AppEvent::FlightSQLQueryResult(query)); | ||
| }); | ||
| } | ||
| // KeyCode::Enter => { |
There was a problem hiding this comment.
I wonder if we can somehow hide it behind an interface (so the pagination code for FlightSQL and local are the same) 🤔
| @@ -0,0 +1,211 @@ | |||
| // Licensed to the Apache Software Foundation (ASF) under one | |||
(#141) was becoming hard for me to reason about because FlightSQL pagination has to be handled differently than SQL tab pagination (because on the SQL tab we can control batch size directly on the context but we cant do that with FlightSQL). So I would like to split pagination on each of those tabs into their own PRs to keep them more focused and easier to review / reason about / etc.
So this PR makes pagination work and adds integration tests for testing pagination. There is still room for improvement in the end to end testing (right now we test some of the implementation details) but at least we have some coverage.