-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add datafusion-json-functions as optional extension #143
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,6 @@ pub mod tabs; | |
|
||
use crate::app::state::tabs::sql::SQLTabState; | ||
use crate::app::ui::SelectedTab; | ||
use crate::config::get_data_dir; | ||
use log::{debug, error, info}; | ||
use std::path::PathBuf; | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
pub sql_tab: SQLTabState<'app>, | ||
#[cfg(feature = "flightsql")] | ||
pub flightsql_tab: FlightSQLTabState<'app>, | ||
|
@@ -57,7 +55,6 @@ pub struct AppState<'app> { | |
|
||
pub fn initialize<'app>(config_path: PathBuf) -> AppState<'app> { | ||
debug!("Initializing state"); | ||
let data_dir = get_data_dir(); | ||
debug!("Config path: {:?}", config_path); | ||
let config = if config_path.exists() { | ||
debug!("Config exists"); | ||
|
@@ -82,24 +79,28 @@ pub fn initialize<'app>(config_path: PathBuf) -> AppState<'app> { | |
debug!("No config, using default"); | ||
AppConfig::default() | ||
}; | ||
AppState::new(config) | ||
} | ||
|
||
let tabs = Tabs::default(); | ||
|
||
let sql_tab_state = SQLTabState::new(); | ||
#[cfg(feature = "flightsql")] | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just refactored the second half of this function into |
||
pub fn new(config: AppConfig) -> Self { | ||
let tabs = Tabs::default(); | ||
|
||
AppState { | ||
config, | ||
data_dir, | ||
tabs, | ||
sql_tab: sql_tab_state, | ||
let sql_tab_state = SQLTabState::new(); | ||
#[cfg(feature = "flightsql")] | ||
flightsql_tab: flightsql_tab_state, | ||
logs_tab: logs_tab_state, | ||
history_tab: history_tab_state, | ||
should_quit: false, | ||
let flightsql_tab_state = FlightSQLTabState::new(); | ||
let logs_tab_state = LogsTabState::default(); | ||
let history_tab_state = HistoryTabState::default(); | ||
|
||
AppState { | ||
config, | ||
tabs, | ||
sql_tab: sql_tab_state, | ||
#[cfg(feature = "flightsql")] | ||
flightsql_tab: flightsql_tab_state, | ||
logs_tab: logs_tab_state, | ||
history_tab: history_tab_state, | ||
should_quit: false, | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
// Licensed to the Apache Software Foundation (ASF) under one | ||
// or more contributor license agreements. See the NOTICE file | ||
// distributed with this work for additional information | ||
// regarding copyright ownership. The ASF licenses this file | ||
// to you under the Apache License, Version 2.0 (the | ||
// "License"); you may not use this file except in compliance | ||
// with the License. You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, | ||
// software distributed under the License is distributed on an | ||
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
// KIND, either express or implied. See the License for the | ||
// specific language governing permissions and limitations | ||
// under the License. | ||
|
||
//! [datafusion-function-json] Integration: [JsonFunctionsExtension] | ||
//! | ||
//! [datafusion-function-json]: https://github.com/datafusion-contrib/datafusion-functions-json | ||
|
||
use crate::config::ExecutionConfig; | ||
use crate::extensions::{DftSessionStateBuilder, Extension}; | ||
use datafusion::prelude::SessionContext; | ||
use datafusion_common::Result; | ||
|
||
#[derive(Debug, Default)] | ||
pub struct JsonFunctionsExtension {} | ||
|
||
impl JsonFunctionsExtension { | ||
pub fn new() -> Self { | ||
Self {} | ||
} | ||
} | ||
|
||
impl Extension for JsonFunctionsExtension { | ||
fn register( | ||
&self, | ||
_config: &ExecutionConfig, | ||
builder: DftSessionStateBuilder, | ||
) -> datafusion_common::Result<DftSessionStateBuilder> { | ||
// | ||
Ok(builder) | ||
} | ||
|
||
fn register_on_ctx(&self, _config: &ExecutionConfig, ctx: &mut SessionContext) -> Result<()> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
datafusion_functions_json::register_all(ctx)?; | ||
Ok(()) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
// Licensed to the Apache Software Foundation (ASF) under one | ||
// or more contributor license agreements. See the NOTICE file | ||
// distributed with this work for additional information | ||
// regarding copyright ownership. The ASF licenses this file | ||
// to you under the Apache License, Version 2.0 (the | ||
// "License"); you may not use this file except in compliance | ||
// with the License. You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, | ||
// software distributed under the License is distributed on an | ||
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
// KIND, either express or implied. See the License for the | ||
// specific language governing permissions and limitations | ||
// under the License. | ||
|
||
//! Tests for datafusion-function-json integration | ||
|
||
use crate::TestExecution; | ||
|
||
static TEST_TABLE: &str = r#" | ||
CREATE TABLE test_table ( | ||
id INT, | ||
json_col VARCHAR | ||
) AS VALUES | ||
(1, '{}'), | ||
(2, '{ "a": 1 }'), | ||
(3, '{ "a": 2 }'), | ||
(4, '{ "a": 1, "b": 2 }'), | ||
(5, '{ "a": 1, "b": 2, "c": 3 }') | ||
"#; | ||
|
||
/// Ensure one of the functions `json_contains` function is properly registered | ||
#[tokio::test] | ||
async fn test_basic() { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am quite pleased this works so well! |
||
.await; | ||
|
||
insta::assert_yaml_snapshot!(actual, @r###" | ||
- +----+---------------+ | ||
- "| id | json_contains |" | ||
- +----+---------------+ | ||
- "| 1 | false |" | ||
- "| 2 | false |" | ||
- "| 3 | false |" | ||
- "| 4 | true |" | ||
- "| 5 | true |" | ||
- +----+---------------+ | ||
"###); | ||
} | ||
Comment on lines
+43
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have not used There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My TLDR is:
b is the game changer -- you can update a bunch of tests simply by running There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That sounds pretty cool! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is like |
||
|
||
/// ensure the json operators like -> are properly registered | ||
#[tokio::test] | ||
async fn test_operators() { | ||
let mut execution = TestExecution::new().with_setup(TEST_TABLE).await; | ||
|
||
let actual = execution | ||
.run_and_format("SELECT id, json_col->'a' as json_col_a FROM test_table") | ||
.await; | ||
|
||
insta::assert_yaml_snapshot!(actual, @r###" | ||
- +----+------------+ | ||
- "| id | json_col_a |" | ||
- +----+------------+ | ||
- "| 1 | {null=} |" | ||
- "| 2 | {int=1} |" | ||
- "| 3 | {int=2} |" | ||
- "| 4 | {int=1} |" | ||
- "| 5 | {int=1} |" | ||
- +----+------------+ | ||
"###); | ||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this test organization |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
// Licensed to the Apache Software Foundation (ASF) under one | ||
// or more contributor license agreements. See the NOTICE file | ||
// distributed with this work for additional information | ||
// regarding copyright ownership. The ASF licenses this file | ||
// to you under the Apache License, Version 2.0 (the | ||
// "License"); you may not use this file except in compliance | ||
// with the License. You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, | ||
// software distributed under the License is distributed on an | ||
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
// KIND, either express or implied. See the License for the | ||
// specific language governing permissions and limitations | ||
// under the License. | ||
|
||
#[cfg(feature = "functions-json")] | ||
mod functions_json; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
)