Add datafusion-json-functions as optional extension #143
Conversation
| Ok(builder) | ||
| } | ||
|
|
||
| fn register_on_ctx(&self, _config: &ExecutionConfig, ctx: &mut SessionContext) -> Result<()> { |
There was a problem hiding this comment.
The main API listed on https://crates.io/crates/datafusion-functions-json/0.41.0 is in terms of SessionContext so I needed to add this
I think this experience trying to configure DataFusion externally has been a good one for me, as it is clearer how awkward it can be (not impossible, but a bit annoying). I am looking forward to bringing some of this learning back to the core with nicer APIs
| //! Tests for datafusion-function-json integration | ||
|
|
||
| #[test] | ||
| fn test_basic() {} |
There was a problem hiding this comment.
I need to write some basic tests here
alamb
force-pushed
the
alamb/json_functions
branch
from
c221a99 to
a3099ee
Compare
| @@ -43,13 +44,15 @@ url = { version = "2.5.2", optional = true } | |||
|
|
|||
| [dev-dependencies] | |||
| assert_cmd = "2.0.16" | |||
| insta = { version = "1.40.0", features = ["yaml"] } | |||
There was a problem hiding this comment.
I want to propose using https://insta.rs/ for testing -- I have found it to be quite useful (it basically automates the updating of expected output by running cargo insta review)
| @@ -46,7 +45,6 @@ impl Default for Tabs { | |||
| pub struct AppState<'app> { | |||
| pub config: AppConfig, | |||
| pub should_quit: bool, | |||
| pub data_dir: PathBuf, | |||
There was a problem hiding this comment.
This field is not used, and I was trying to reduce the overhead of running tests (e.g. not open files if it isn't necessary)
There was a problem hiding this comment.
I just double checked and that field was from the Polygon TUI I was working on and specific to that app - so good to remove
| let flightsql_tab_state = FlightSQLTabState::new(); | ||
| let logs_tab_state = LogsTabState::default(); | ||
| let history_tab_state = HistoryTabState::default(); | ||
| impl<'app> AppState<'app> { |
There was a problem hiding this comment.
I just refactored the second half of this function into AppState::new() so I could pass in a AppConfig rather than having to create one from a directory
| let mut execution = TestExecution::new().with_setup(TEST_TABLE).await; | ||
|
|
||
| let actual = execution | ||
| .run_and_format("SELECT id, json_contains(json_col, 'b') as json_contains FROM test_table") |
There was a problem hiding this comment.
I am quite pleased this works so well!
alamb
force-pushed
the
alamb/json_functions
branch
from
9efba90 to
f047cc4
Compare
|
|
||
| mod extension_cases; | ||
|
|
||
| /// Encapsulates an `ExecutionContext` for running queries in tests |
There was a problem hiding this comment.
I am hoping to use this same structure to run tests for most of the other integration setups.
|
I also filed a PR to improve the json docs: datafusion-contrib/datafusion-functions-json#45 |
There was a problem hiding this comment.
I like this test organization
| insta::assert_yaml_snapshot!(actual, @r###" | ||
| - +----+---------------+ | ||
| - "| id | json_contains |" | ||
| - +----+---------------+ | ||
| - "| 1 | false |" | ||
| - "| 2 | false |" | ||
| - "| 3 | false |" | ||
| - "| 4 | true |" | ||
| - "| 5 | true |" | ||
| - +----+---------------+ | ||
| "###); | ||
| } |
There was a problem hiding this comment.
I have not used insta before so i find this a little confusing (in particular its not clear to me what the "yaml" is doing). But I have heard good things about insta so will start looking into it.
There was a problem hiding this comment.
My TLDR is:
- Just follow the model (I don't really know what
assert_yaml_snapshotdoes internally) - When the test fails you run
cargo instawhich will a) show you a diff and b) offer to update the expected text for you.
b is the game changer -- you can update a bunch of tests simply by running cargo insta and typing a after looking at the diffs
There was a problem hiding this comment.
That sounds pretty cool!
There was a problem hiding this comment.
It is like sqllogictest --complete mode but for rust unit tests 🚀
Closes #130
Changes: