diff --git a/.github/workflows/test-unit.yml b/.github/workflows/test-unit.yml index 3497d0056a2..7c1a8ecb406 100644 --- a/.github/workflows/test-unit.yml +++ b/.github/workflows/test-unit.yml @@ -36,16 +36,20 @@ jobs: uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 - name: Download Test Binaries - uses: actions/download-artifact@37930b1c2abaa49bbe596cd826c3c89aef350131 # v7 + uses: dawidd6/action-download-artifact@b6e2e70617bc3265edd6dab6c906732b2f1ae151 # v21 with: name: feldera-test-binaries-${{ matrix.target }} path: build + run_id: ${{ github.run_id }} + github_token: ${{ secrets.GITHUB_TOKEN }} - name: Download Compiler Binaries - uses: actions/download-artifact@37930b1c2abaa49bbe596cd826c3c89aef350131 # v7 + uses: dawidd6/action-download-artifact@b6e2e70617bc3265edd6dab6c906732b2f1ae151 # v21 with: name: feldera-sql-compiler path: sql-build + run_id: ${{ github.run_id }} + github_token: ${{ secrets.GITHUB_TOKEN }} # Remove if https://github.com/actions/upload-artifact/issues/38 ever gets fixed - name: Make binaries executable diff --git a/Cargo.lock b/Cargo.lock index 9f3a18bee29..2cff97c192b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3952,7 +3952,7 @@ dependencies = [ [[package]] name = "dbsp" -version = "0.316.0" +version = "0.317.0" dependencies = [ "anyhow", "arc-swap", @@ -4042,7 +4042,7 @@ dependencies = [ [[package]] name = "dbsp_adapters" -version = "0.316.0" +version = "0.317.0" dependencies = [ "actix", "actix-codec", @@ -4186,7 +4186,7 @@ dependencies = [ [[package]] name = "dbsp_nexmark" -version = "0.316.0" +version = "0.317.0" dependencies = [ "anyhow", "ascii_table", @@ -5170,7 +5170,7 @@ dependencies = [ [[package]] name = "fda" -version = "0.316.0" +version = "0.317.0" dependencies = [ "anyhow", "arrow", @@ -5222,7 +5222,7 @@ dependencies = [ [[package]] name = "feldera-adapterlib" -version = "0.316.0" +version = "0.317.0" dependencies = [ "actix-web", "anyhow", @@ -5256,7 +5256,7 @@ dependencies = [ [[package]] name = "feldera-buffer-cache" -version = "0.316.0" +version = "0.317.0" dependencies = [ "crossbeam-utils", "enum-map", @@ -5284,7 +5284,7 @@ dependencies = [ [[package]] name = "feldera-datagen" -version = "0.316.0" +version = "0.317.0" dependencies = [ "anyhow", "async-channel 2.5.0", @@ -5310,7 +5310,7 @@ dependencies = [ [[package]] name = "feldera-fxp" -version = "0.316.0" +version = "0.317.0" dependencies = [ "bytecheck", "dbsp", @@ -5330,7 +5330,7 @@ dependencies = [ [[package]] name = "feldera-iceberg" -version = "0.316.0" +version = "0.317.0" dependencies = [ "anyhow", "chrono", @@ -5352,7 +5352,7 @@ dependencies = [ [[package]] name = "feldera-ir" -version = "0.316.0" +version = "0.317.0" dependencies = [ "proptest", "proptest-derive", @@ -5364,7 +5364,7 @@ dependencies = [ [[package]] name = "feldera-macros" -version = "0.316.0" +version = "0.317.0" dependencies = [ "prettyplease", "proc-macro2", @@ -5374,7 +5374,7 @@ dependencies = [ [[package]] name = "feldera-observability" -version = "0.316.0" +version = "0.317.0" dependencies = [ "actix-http", "awc", @@ -5389,7 +5389,7 @@ dependencies = [ [[package]] name = "feldera-rest-api" -version = "0.316.0" +version = "0.317.0" dependencies = [ "chrono", "feldera-observability", @@ -5408,7 +5408,7 @@ dependencies = [ [[package]] name = "feldera-samply" -version = "0.316.0" +version = "0.317.0" dependencies = [ "crossbeam", "feldera-size-of", @@ -5444,7 +5444,7 @@ dependencies = [ [[package]] name = "feldera-sqllib" -version = "0.316.0" +version = "0.317.0" dependencies = [ "arcstr", "base58", @@ -5488,7 +5488,7 @@ dependencies = [ [[package]] name = "feldera-storage" -version = "0.316.0" +version = "0.317.0" dependencies = [ "anyhow", "crossbeam", @@ -5511,7 +5511,7 @@ dependencies = [ [[package]] name = "feldera-types" -version = "0.316.0" +version = "0.317.0" dependencies = [ "actix-web", "anyhow", @@ -8774,7 +8774,7 @@ checksum = "8b870d8c151b6f2fb93e84a13146138f05d02ed11c7e7c54f8826aaaf7c9f184" [[package]] name = "pipeline-manager" -version = "0.316.0" +version = "0.317.0" dependencies = [ "actix-cors", "actix-files", @@ -9984,7 +9984,7 @@ dependencies = [ [[package]] name = "readers" -version = "0.316.0" +version = "0.317.0" dependencies = [ "async-std", "csv", @@ -11683,7 +11683,7 @@ checksum = "0c790de23124f9ab44544d7ac05d60440adc586479ce501c1d6d7da3cd8c9cf5" [[package]] name = "sltsqlvalue" -version = "0.316.0" +version = "0.317.0" dependencies = [ "dbsp", "feldera-sqllib", @@ -12154,7 +12154,7 @@ checksum = "a2eb9349b6444b326872e140eb1cf5e7c522154d69e7a0ffb0fb81c06b37543f" [[package]] name = "storage-test-compat" -version = "0.316.0" +version = "0.317.0" dependencies = [ "dbsp", "derive_more 1.0.0", diff --git a/Cargo.toml b/Cargo.toml index 953fab47c5e..fe00c686930 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [workspace.package] authors = ["Feldera Team "] -version = "0.316.0" +version = "0.317.0" license = "MIT OR Apache-2.0" homepage = "https://github.com/feldera/feldera" repository = "https://github.com/feldera/feldera" @@ -108,7 +108,7 @@ csv = "1.2.2" csv-core = "0.1.10" dashmap = "6.1.0" datafusion = "53.1" -dbsp = { path = "crates/dbsp", version = "0.316.0" } +dbsp = { path = "crates/dbsp", version = "0.317.0" } dbsp_nexmark = { path = "crates/nexmark" } deadpool-postgres = "0.14.1" # Feldera fork of delta-rs: upstream tag `rust-v0.32.3` + 2 patches, on branch @@ -133,20 +133,20 @@ erased-serde = "0.3.31" fake = "2.10" fastbloom = "0.14.0" fdlimit = "0.3.0" -feldera-buffer-cache = { version = "0.316.0", path = "crates/buffer-cache" } +feldera-buffer-cache = { version = "0.317.0", path = "crates/buffer-cache" } feldera-cloud1-client = "0.1.2" feldera-datagen = { path = "crates/datagen" } -feldera-fxp = { version = "0.316.0", path = "crates/fxp", features = ["dbsp"] } +feldera-fxp = { version = "0.317.0", path = "crates/fxp", features = ["dbsp"] } feldera-iceberg = { path = "crates/iceberg" } -feldera-observability = { version = "0.316.0", path = "crates/feldera-observability" } -feldera-macros = { version = "0.316.0", path = "crates/feldera-macros" } -feldera-sqllib = { version = "0.316.0", path = "crates/sqllib" } -feldera-storage = { version = "0.316.0", path = "crates/storage" } -feldera-types = { version = "0.316.0", path = "crates/feldera-types" } -feldera-rest-api = { version = "0.316.0", path = "crates/rest-api" } -feldera-ir = { version = "0.316.0", path = "crates/ir" } -feldera-adapterlib = { version = "0.316.0", path = "crates/adapterlib" } -feldera-samply = { version = "0.316.0", path = "crates/samply" } +feldera-observability = { version = "0.317.0", path = "crates/feldera-observability" } +feldera-macros = { version = "0.317.0", path = "crates/feldera-macros" } +feldera-sqllib = { version = "0.317.0", path = "crates/sqllib" } +feldera-storage = { version = "0.317.0", path = "crates/storage" } +feldera-types = { version = "0.317.0", path = "crates/feldera-types" } +feldera-rest-api = { version = "0.317.0", path = "crates/rest-api" } +feldera-ir = { version = "0.317.0", path = "crates/ir" } +feldera-adapterlib = { version = "0.317.0", path = "crates/adapterlib" } +feldera-samply = { version = "0.317.0", path = "crates/samply" } flate2 = "1.1.0" form_urlencoded = "1.2.0" fs_extra = "1.3.0" diff --git a/crates/adapters/src/controller.rs b/crates/adapters/src/controller.rs index bf7a6056831..298f14760b1 100644 --- a/crates/adapters/src/controller.rs +++ b/crates/adapters/src/controller.rs @@ -4434,31 +4434,36 @@ impl FtState { let mut changed_inputs = HashMap::new(); let inputs = self.controller.status.input_status(); - // Stop recording if the controller is shutting down. Avoid race with - // `stop`, which removes input endpoints from the pipeline. Without this - // check we may end up recording these endpoints in `remove_inputs`. - if self.controller.state() == PipelineState::Terminated { - return Ok(()); - } - self.input_endpoints - .retain(|endpoint_id, (endpoint_name, paused)| { - if let Some(endpoint) = inputs.get(endpoint_id) { - let now_paused = endpoint.is_paused_by_user(); - if *paused != now_paused { - changed_inputs.insert(endpoint_name.clone(), now_paused); - *paused = now_paused; + // Compute the endpoint lifecycle diff (added/removed/paused since + // the previous step) that this step's journal entry records, but + // skip it while the controller is shutting down: `stop` + // concurrently disconnects endpoints, so the diff would spuriously + // record still-live endpoints as removed and replay them away. + // + // We must still journal this step's already-computed input, or a + // clean stop would silently drop the last step and a subsequent + // restart would resume from before it. + if self.controller.state() != PipelineState::Terminated { + self.input_endpoints + .retain(|endpoint_id, (endpoint_name, paused)| { + if let Some(endpoint) = inputs.get(endpoint_id) { + let now_paused = endpoint.is_paused_by_user(); + if *paused != now_paused { + changed_inputs.insert(endpoint_name.clone(), now_paused); + *paused = now_paused; + } + true + } else { + remove_inputs.insert(endpoint_name.clone()); + false } - true - } else { - remove_inputs.insert(endpoint_name.clone()); - false - } - }); - for (endpoint_id, status) in inputs.iter() { - self.input_endpoints.entry(*endpoint_id).or_insert_with(|| { - add_inputs.insert(status.endpoint_name.clone(), status.config.clone()); - (status.endpoint_name.clone(), status.is_paused_by_user()) - }); + }); + for (endpoint_id, status) in inputs.iter() { + self.input_endpoints.entry(*endpoint_id).or_insert_with(|| { + add_inputs.insert(status.endpoint_name.clone(), status.config.clone()); + (status.endpoint_name.clone(), status.is_paused_by_user()) + }); + } } drop(inputs); @@ -5031,20 +5036,6 @@ impl ControllerInit { ) } - // Transfer HTTP input endpoints that are not affected by the program diff from the checkpoint to the new configuration. - checkpoint_config - .inputs - .iter() - .filter(|(_connector_name, connector_config)| { - connector_config.connector_config.transport.is_http_input() - && !pipeline_diff.is_affected_relation(&connector_config.stream) - }) - .for_each(|(connector_name, connector_config)| { - config - .inputs - .insert(connector_name.clone(), connector_config.clone()); - }); - // Merge `config` (the configuration provided by the pipeline manager) // with `checkpoint_config` (the configuration read from the // checkpoint). @@ -5097,26 +5088,11 @@ impl ControllerInit { pipeline_template_configmap: config.global.pipeline_template_configmap.clone(), }, - // If pipeline is unmodified, we may need to replay journaled inputs. - // We therefore use connector configuration from the checkpoint, including - // transient HTTP and adhoc connectors, so that we can use them to - // replay journaled inputs. - inputs: if !modified { - checkpoint_config - .inputs - .into_iter() - .filter(|(_, config)| { - // The clock input connector will be automatically recreated and initialized - // with the clock resolution from the pipeline config. - !matches!( - config.connector_config.transport, - TransportConfig::ClockInput(_) - ) - }) - .collect() - } else { - config.inputs - }, + inputs: Self::inputs_for_checkpoint_replay( + config.inputs, + checkpoint_config.inputs, + &pipeline_diff, + ), outputs: if !modified { checkpoint_config.outputs } else { @@ -5148,6 +5124,69 @@ impl ControllerInit { }) } + fn inputs_for_checkpoint_replay( + mut config_inputs: BTreeMap, InputEndpointConfig>, + checkpoint_inputs: BTreeMap, InputEndpointConfig>, + pipeline_diff: &feldera_types::pipeline_diff::PipelineDiff, + ) -> BTreeMap, InputEndpointConfig> { + // Preserve the pipeline manager's input configs before HTTP input + // endpoints are transferred from the checkpoint below. The final + // replay config still needs replay-safe limits from the new config. + let new_inputs = config_inputs.clone(); + + // Transfer HTTP input endpoints that are not affected by the program + // diff from the checkpoint to the new configuration. + checkpoint_inputs + .iter() + .filter(|(_connector_name, connector_config)| { + connector_config.connector_config.transport.is_http_input() + && !pipeline_diff.is_affected_relation(&connector_config.stream) + }) + .for_each(|(connector_name, connector_config)| { + config_inputs.insert(connector_name.clone(), connector_config.clone()); + }); + + if pipeline_diff.is_empty() { + // If the pipeline is unmodified, we may need to replay journaled + // inputs. We therefore use connector configuration from the + // checkpoint, including transient HTTP and adhoc connectors, so + // that we can use them to replay journaled inputs. Input + // flow-control settings are safe to adopt from the new config + // without invalidating checkpointed state. + Self::checkpoint_inputs_with_replay_safe_config(checkpoint_inputs, &new_inputs) + } else { + // Otherwise, keep the new pipeline-manager config, including + // transferred HTTP and adhoc inputs needed to replay journaled + // inputs after a program-diff bootstrap. + config_inputs + } + } + + fn checkpoint_inputs_with_replay_safe_config( + mut checkpoint_inputs: BTreeMap, InputEndpointConfig>, + new_inputs: &BTreeMap, InputEndpointConfig>, + ) -> BTreeMap, InputEndpointConfig> { + for (connector_name, checkpoint_input) in checkpoint_inputs.iter_mut() { + if let Some(new_input) = new_inputs.get(connector_name) { + checkpoint_input + .connector_config + .apply_input_checkpoint_replay_config_from(&new_input.connector_config); + } + } + + checkpoint_inputs + .into_iter() + .filter(|(_, config)| { + // The clock input connector will be automatically recreated and initialized + // with the clock resolution from the pipeline config. + !matches!( + config.connector_config.transport, + TransportConfig::ClockInput(_) + ) + }) + .collect() + } + pub fn set_incarnation_uuid(&mut self, incarnation_uuid: Uuid) { self.incarnation_uuid = incarnation_uuid } @@ -8766,7 +8805,10 @@ mod controller_init_tests { use super::ControllerInit; use crate::ControllerError; use feldera_adapterlib::errors::controller::ConfigError; - use feldera_types::config::{PipelineConfig, RuntimeConfig}; + use feldera_types::config::{InputEndpointConfig, PipelineConfig, RuntimeConfig}; + use feldera_types::pipeline_diff::PipelineDiff; + use serde_json::json; + use std::{borrow::Cow, collections::BTreeMap}; fn pipeline_config(global: RuntimeConfig) -> PipelineConfig { PipelineConfig { @@ -8830,7 +8872,85 @@ mod controller_init_tests { if matches!( *config_error, ConfigError::DatafusionMemoryExceedsBudget { .. }, - ), + ), )); } + + #[test] + fn checkpoint_inputs_adopt_replay_safe_config() { + let mut checkpoint_inputs = BTreeMap::new(); + checkpoint_inputs.insert( + Cow::Borrowed("test_input.connector"), + input_config(json!({"name": "empty_input"}), 100), + ); + + let mut new_inputs = BTreeMap::new(); + new_inputs.insert( + Cow::Borrowed("test_input.connector"), + input_config(json!({"name": "empty_input"}), 200), + ); + + let merged = ControllerInit::checkpoint_inputs_with_replay_safe_config( + checkpoint_inputs, + &new_inputs, + ); + let connector_config = &merged["test_input.connector"].connector_config; + + assert_eq!(connector_config.max_queued_records, 200); + assert_eq!(connector_config.max_queued_bytes, Some(400)); + assert_eq!(connector_config.max_batch_size, Some(201)); + assert_eq!(connector_config.max_worker_batch_size, Some(202)); + } + + #[test] + fn checkpoint_http_input_replay_uses_pre_transfer_config() { + let mut checkpoint_inputs = BTreeMap::new(); + checkpoint_inputs.insert( + Cow::Borrowed("test_input.connector"), + input_config( + json!({ + "name": "http_input", + "config": {"name": "test_input.connector"} + }), + 100, + ), + ); + + let mut config_inputs = BTreeMap::new(); + config_inputs.insert( + Cow::Borrowed("test_input.connector"), + input_config( + json!({ + "name": "http_input", + "config": {"name": "test_input.connector"} + }), + 200, + ), + ); + + let merged = ControllerInit::inputs_for_checkpoint_replay( + config_inputs, + checkpoint_inputs, + &PipelineDiff::new_with_program_diff(Default::default()), + ); + let connector_config = &merged["test_input.connector"].connector_config; + + assert!(connector_config.transport.is_http_input()); + assert_eq!(connector_config.max_queued_records, 200); + assert_eq!(connector_config.max_queued_bytes, Some(400)); + assert_eq!(connector_config.max_batch_size, Some(201)); + assert_eq!(connector_config.max_worker_batch_size, Some(202)); + } + + fn input_config(transport: serde_json::Value, max_queued_records: u64) -> InputEndpointConfig { + serde_json::from_value(json!({ + "stream": "test_input", + "transport": transport, + "max_queued_records": max_queued_records, + "max_queued_bytes": max_queued_records * 2, + "max_batch_size": max_queued_records + 1, + "max_worker_batch_size": max_queued_records + 2, + })) + .unwrap() + } } diff --git a/crates/adapters/src/controller/pipeline_diff.rs b/crates/adapters/src/controller/pipeline_diff.rs index f9e5ead96bf..fef329127f0 100644 --- a/crates/adapters/src/controller/pipeline_diff.rs +++ b/crates/adapters/src/controller/pipeline_diff.rs @@ -147,7 +147,7 @@ pub fn compute_pipeline_diff( .get(*k) .unwrap() .connector_config - .equal_modulo_paused(&v.connector_config) + .equal_for_input_checkpoint_replay(&v.connector_config) }) .map(|(k, _)| k.to_string()) .collect::>(); @@ -197,3 +197,67 @@ pub fn compute_pipeline_diff( .with_modified_output_connectors(modified_output_connectors) .with_removed_output_connectors(removed_output_connectors)) } + +#[cfg(test)] +mod tests { + use super::compute_pipeline_diff; + use feldera_types::config::PipelineConfig; + use serde_json::{Map, Value, json}; + + fn input_endpoint(mut fields: Map) -> Value { + let mut endpoint = Map::from_iter([ + ("stream".to_string(), json!("t1")), + ("transport".to_string(), json!({"name": "empty_input"})), + ]); + endpoint.append(&mut fields); + Value::Object(endpoint) + } + + fn pipeline_config(input: Value) -> PipelineConfig { + serde_json::from_value(json!({ + "name": "test", + "workers": 1, + "inputs": { + "t1.connector": input + } + })) + .unwrap() + } + + #[test] + fn input_flow_control_changes_do_not_modify_connector() { + let old_config = pipeline_config(input_endpoint(Map::new())); + let new_config = pipeline_config(input_endpoint(Map::from_iter([ + ("max_queued_records".to_string(), json!(5000)), + ("max_queued_bytes".to_string(), json!(6000)), + ("max_batch_size".to_string(), json!(7000)), + ("max_worker_batch_size".to_string(), json!(8000)), + ("paused".to_string(), json!(true)), + ]))); + + let diff = compute_pipeline_diff(&old_config, &new_config).unwrap(); + + assert!(diff.modified_input_connectors().is_empty()); + } + + #[test] + fn input_transport_changes_still_modify_connector() { + let old_config = pipeline_config(input_endpoint(Map::new())); + let new_config = pipeline_config(input_endpoint(Map::from_iter([( + "transport".to_string(), + json!({ + "name": "datagen", + "config": { + "plan": [{"limit": 1}] + } + }), + )]))); + + let diff = compute_pipeline_diff(&old_config, &new_config).unwrap(); + + assert_eq!( + diff.modified_input_connectors(), + &vec!["t1.connector".to_string()] + ); + } +} diff --git a/crates/adapters/src/controller/test.rs b/crates/adapters/src/controller/test.rs index 8663c9e5705..6f2e34368a9 100644 --- a/crates/adapters/src/controller/test.rs +++ b/crates/adapters/src/controller/test.rs @@ -2062,7 +2062,7 @@ fn suspend_barrier() { // Suspend should now succeed, because we crossed the barrier. receiver - .recv_timeout(Duration::from_millis(10000)) + .recv_timeout(Duration::from_millis(100000)) .unwrap() .unwrap(); assert_bounded_suspend_steps(&controller, suspend_request_step); @@ -2113,7 +2113,7 @@ fn suspend_barrier() { // Suspend should now succeed, because we crossed the barrier. receiver - .recv_timeout(Duration::from_millis(10000)) + .recv_timeout(Duration::from_millis(100000)) .unwrap() .unwrap(); } @@ -2238,7 +2238,7 @@ fn suspend_multiple_barriers(n_inputs: usize) { // Suspend should now succeed, because we crossed the barrier. receiver - .recv_timeout(Duration::from_millis(10000)) + .recv_timeout(Duration::from_millis(100000)) .unwrap() .unwrap(); assert_bounded_suspend_steps(&controller, suspend_request_step); diff --git a/crates/adapters/src/integrated/postgres/test.rs b/crates/adapters/src/integrated/postgres/test.rs index 8163cec2d19..533cf65759c 100644 --- a/crates/adapters/src/integrated/postgres/test.rs +++ b/crates/adapters/src/integrated/postgres/test.rs @@ -3092,6 +3092,7 @@ mod cdc_tests { /// Requires: wal_level=logical, user with REPLICATION privilege. #[test] #[serial] + #[ignore] fn test_cdc_basic_insert() { let url = postgres_url(); let table_name = unique_pg_name("cdc_test_basic_insert"); @@ -3173,6 +3174,7 @@ mod cdc_tests { /// Requires: wal_level=logical, user with REPLICATION privilege. #[test] #[serial] + #[ignore] fn test_cdc_pause_unpause() { let url = postgres_url(); let table_name = unique_pg_name("cdc_test_pause_unpause"); @@ -3330,6 +3332,7 @@ mod cdc_tests { /// Requires: wal_level=logical, user with REPLICATION privilege. #[test] #[serial] + #[ignore] fn test_cdc_all_data_types() { let url = postgres_url(); let table_name = unique_pg_name("cdc_test_all_types"); @@ -3474,6 +3477,7 @@ mod cdc_tests { /// Requires: wal_level=logical, user with REPLICATION privilege, REPLICA IDENTITY FULL. #[test] #[serial] + #[ignore] fn test_cdc_update_delete() { let url = postgres_url(); let table_name = unique_pg_name("cdc_test_upd_del"); @@ -3592,6 +3596,7 @@ mod cdc_tests { /// Requires: wal_level=logical, user with REPLICATION privilege. #[test] #[serial] + #[ignore] fn test_cdc_compatible_schema_changes() { let url = postgres_url(); let table_name = unique_pg_name("cdc_test_compatible_schema"); @@ -3683,6 +3688,7 @@ mod cdc_tests { /// Requires: wal_level=logical, user with REPLICATION privilege. #[test] #[serial] + #[ignore] fn test_cdc_drop_primary_key_column_rejected() { let url = postgres_url(); let table_name = unique_pg_name("cdc_test_drop_pk_col"); @@ -3746,6 +3752,7 @@ mod cdc_tests { /// Requires: wal_level=logical, user with REPLICATION privilege. #[test] #[serial] + #[ignore] fn test_cdc_restart_resumes_from_slot() { let url = postgres_url(); let table_name = unique_pg_name("cdc_test_restart"); @@ -3882,6 +3889,7 @@ mod cdc_tests { /// Requires: wal_level=logical, user with REPLICATION privilege. #[test] #[serial] + #[ignore] fn test_cdc_ft_mode_holds_slot() { let url = postgres_url(); let table_name = unique_pg_name("cdc_test_strict_hold"); diff --git a/crates/adapters/src/transport/clock.rs b/crates/adapters/src/transport/clock.rs index 579c9dec872..f76c0b54d1d 100644 --- a/crates/adapters/src/transport/clock.rs +++ b/crates/adapters/src/transport/clock.rs @@ -891,6 +891,13 @@ mod test { } /// After checkpoint/restart, `NOW()` resumes from the last replayed value, not the anchor. + /// + /// Regression coverage for a fault-tolerance journaling race: a step is + /// journaled after its circuit evaluation (so the entry can carry the + /// transaction id assigned there), and a `stop` landing between a step's + /// output being emitted and its journal write must not drop that step, or + /// the post-checkpoint advance is absent from the replay log and `NOW()` + /// regresses to the checkpoint value on restart. #[test] fn test_clock_advance_survives_checkpoint() { use dbsp::circuit::tokio::TOKIO; @@ -987,6 +994,18 @@ mod test { let reader = controller.get_input_endpoint("now").unwrap(); let clock_reader = reader.as_any().downcast::().unwrap(); + // Replay runs asynchronously on the circuit thread, feeding the journaled + // steps one at a time. Wait for it to finish before reading `NOW()`, or we + // could observe an intermediate value from before the last replayed step. + let deadline = std::time::Instant::now() + Duration::from_secs(30); + while controller.is_replaying() { + assert!( + std::time::Instant::now() < deadline, + "replay did not complete within 30s" + ); + sleep(Duration::from_millis(10)); + } + let post_replay = TOKIO.block_on(clock_reader.advance(Some(0))).unwrap(); assert_eq!( post_replay, last_emitted_in_run1, diff --git a/crates/adapters/src/transport/kafka/ft/test.rs b/crates/adapters/src/transport/kafka/ft/test.rs index d32e7c56ce7..cb2425a83bd 100644 --- a/crates/adapters/src/transport/kafka/ft/test.rs +++ b/crates/adapters/src/transport/kafka/ft/test.rs @@ -757,7 +757,12 @@ impl InputConsumer for DummyInputConsumer { fn error(&self, fatal: bool, error: AnyError, _tag: Option<&'static str>) { info!("error: {error}"); - self.called(ConsumerCall::Error(fatal)); + // Record only fatal errors. Non-fatal errors, such as a momentary + // broker disconnection, can arrive at any time, and recording them + // would break the strict call sequence that `expect()` checks. + if fatal { + self.called(ConsumerCall::Error(fatal)); + } } fn request_step(&self) {} diff --git a/crates/dbsp/src/operator.rs b/crates/dbsp/src/operator.rs index dc7f013f15c..19ff96d359a 100644 --- a/crates/dbsp/src/operator.rs +++ b/crates/dbsp/src/operator.rs @@ -22,6 +22,7 @@ mod csv; mod delta0; mod differentiate; mod generator; +mod integral_weight_validator; mod integrate; mod neg; mod output; diff --git a/crates/dbsp/src/operator/dynamic/balance/balancer.rs b/crates/dbsp/src/operator/dynamic/balance/balancer.rs index 4494ef79b29..c6a1d1ac6b0 100644 --- a/crates/dbsp/src/operator/dynamic/balance/balancer.rs +++ b/crates/dbsp/src/operator/dynamic/balance/balancer.rs @@ -778,26 +778,38 @@ impl BalancerInner { // ); let cluster_index = *self.stream_to_cluster.get(&node_id).unwrap(); - let (_exchange_sender, _persistent_id, hints) = self - .integrals - .get_mut(&node_id) - .ok_or(BalancerError::NotRegisteredWithBalancer(node_id))?; - - let mut hints = hints.clone(); - - match hint { - BalancerHint::Policy(policy) => { - if let Some(policy) = policy { - hints.policy_hint = Some(policy); - self.solve_cluster(cluster_index, false)?; - } else { - hints.policy_hint = None; - } + // Apply the hint to the stored hints, remembering the previous value so we + // can roll back if the hint turns out to be unenforceable. The hint must be + // stored *before* validating below, because `solve_cluster` reads the hint + // from `self.integrals`; validating against the pre-hint state would accept + // hints that make the cluster unsatisfiable and crash at the first step. + let old_hints = { + let (_exchange_sender, _persistent_id, hints) = self + .integrals + .get_mut(&node_id) + .ok_or(BalancerError::NotRegisteredWithBalancer(node_id))?; + + let old_hints = hints.clone(); + match hint { + BalancerHint::Policy(policy) => hints.policy_hint = policy, + BalancerHint::Size(size) => hints.size_hint = size, + BalancerHint::Skew(skew) => hints.skew_hint = skew, } - BalancerHint::Size(size) => hints.size_hint = size, - BalancerHint::Skew(skew) => hints.skew_hint = skew, + old_hints + }; + + if let BalancerHint::Policy(Some(policy)) = hint + && self.solve_cluster(cluster_index, false).is_err() + { + self.integrals.get_mut(&node_id).unwrap().2 = old_hints; + return Err(BalancerError::InvalidPolicyHint( + policy, + format!( + "'{policy}' policy is incompatible with the partitioning constraints of the streams it joins with" + ), + )); } - self.integrals.get_mut(&node_id).unwrap().2 = hints.clone(); + self.clusters[cluster_index].refresh_requested = true; Ok(()) diff --git a/crates/dbsp/src/operator/dynamic/balance/test.rs b/crates/dbsp/src/operator/dynamic/balance/test.rs index a1a795b1736..7c6a83e3196 100644 --- a/crates/dbsp/src/operator/dynamic/balance/test.rs +++ b/crates/dbsp/src/operator/dynamic/balance/test.rs @@ -1147,6 +1147,51 @@ fn test_accumulate_trace_with_balancer_policy_returns_to_trace_policy() { assert_eq!(output_trace, expected_output_trace); } +/// A `Broadcast` policy hint on the outer (left) input of a left join cannot be +/// honored: the left side of a left join must be Shard or Balance. The hint must +/// be rejected when it is installed, rather than accepted and then crashing the +/// circuit with `NoSolution` on the first step. +/// +/// Regression test: `set_hint` used to validate the cluster *before* persisting +/// the hint, so it checked a still-satisfiable (hint-free) instance, accepted the +/// hint, and the first step then solved the now-unsatisfiable instance and +/// panicked in `solve_all_clusters().unwrap()`. This is the crash produced by a +/// SQL `/*+ broadcast(pt) */` hint where `pt` is the outer input of a left join. +#[test] +fn test_reject_broadcast_hint_on_left_join_outer_input() { + init_test_logger(); + + let workers = 4; + let ( + mut circuit, + (left_input_handle, right_input_handle, left_input_node_id, _right_input_node_id, _output), + ) = Runtime::init_circuit( + CircuitConfig::from(workers) + .with_splitter_chunk_size_records(2) + .with_balancer_min_absolute_improvement_threshold(0) + .with_mode(Mode::Persistent), + left_join_with_balancer_test_circuit, + ) + .unwrap(); + + // Broadcasting the outer input of a left join is infeasible, so the hint must + // be rejected here instead of being accepted and crashing at the first step. + let result = circuit.set_balancer_hint( + &left_input_node_id, + BalancerHint::Policy(Some(PartitioningPolicy::Broadcast)), + ); + assert!( + result.is_err(), + "broadcast hint on the outer input of a left join must be rejected, got {result:?}" + ); + + // With the hint rejected and rolled back, the circuit runs normally instead of + // crashing with `NoSolution` on the first step. + left_input_handle.push(1, (1, 1)); + right_input_handle.push(1, (1, 1)); + circuit.transaction().unwrap(); +} + /// Join a large left collection with a small right collection. Both are skewed. /// The balancer should balance the left collection using Policy::Balance and the /// right collection using Policy::Broadcast. diff --git a/crates/dbsp/src/operator/dynamic/recursive.rs b/crates/dbsp/src/operator/dynamic/recursive.rs index 4888b106b26..1af7c633a23 100644 --- a/crates/dbsp/src/operator/dynamic/recursive.rs +++ b/crates/dbsp/src/operator/dynamic/recursive.rs @@ -82,8 +82,8 @@ where ) } - fn connect(&self, vars: Self::Feedback) { - vars.connect(self) + fn connect(&self, var: Self::Feedback) { + var.connect(self) } fn export(self, factories: &Self::Factories) -> Self::Export { @@ -95,7 +95,81 @@ where } } -// TODO: `impl RecursiveStreams for Vec`. +/// Recursion over a group of streams whose size is only known at runtime. +/// +/// The arity of the group (the number of mutually recursive streams) is +/// determined by the length of the `factories` vector passed to +/// [`new`](RecursiveStreams::new). Every other method preserves this arity: +/// the closure driving the recursion must therefore return exactly as many +/// streams as it received. Unlike the tuple implementations, all streams in +/// the group share the same batch type `B`. +impl RecursiveStreams for Vec> +where + C: Circuit, + C::Parent: Circuit, + B: Checkpoint + IndexedZSet + Send + Sync, + Spine: SizeOf, +{ + type Feedback = Vec>; + type Export = Vec>>; + type Output = Vec>; + type Factories = Vec>; + + fn new(circuit: &C, factories: &Self::Factories) -> (Self::Feedback, Self) { + factories + .iter() + .map(|factory| { + let feedback = + DelayedFeedback::with_default(circuit, B::dyn_empty(&factory.input_factories)); + let stream = feedback.stream().clone(); + (feedback, stream) + }) + .unzip() + } + + fn distinct(mut self, factories: &Self::Factories) -> Self { + assert_eq!(self.len(), factories.len()); + + for (stream, factory) in self.iter_mut().zip(factories) { + let persistent_id = stream + .get_persistent_id() + .map(|name| format!("{name}.distinct")); + *stream = + Stream::dyn_distinct(stream, factory).set_persistent_id(persistent_id.as_deref()); + } + + self + } + + fn connect(&self, vars: Self::Feedback) { + assert_eq!(self.len(), vars.len()); + + for (stream, var) in self.iter().zip(vars) { + var.connect(stream); + } + } + + fn export(self, factories: &Self::Factories) -> Self::Export { + assert_eq!(self.len(), factories.len()); + + self.into_iter() + .zip(factories) + .map(|(stream, factory)| { + Stream::export(&stream.dyn_integrate_trace(&factory.input_factories)) + }) + .collect() + } + + fn consolidate(exports: Self::Export, factories: &Self::Factories) -> Self::Output { + assert_eq!(exports.len(), factories.len()); + + exports + .into_iter() + .zip(factories) + .map(|(stream, factory)| Stream::dyn_consolidate(&stream, &factory.input_factories)) + .collect() + } +} #[allow(clippy::unused_unit)] #[impl_for_tuples(14)] @@ -187,8 +261,7 @@ where #[cfg(test)] mod test { use crate::{ - Circuit, FallbackZSet, Runtime, Stream, operator::Generator, typed_batch::OrdZSet, - utils::Tup2, zset, + Circuit, Runtime, Stream, operator::Generator, typed_batch::OrdZSet, utils::Tup2, zset, }; use std::{ thread, @@ -196,64 +269,6 @@ mod test { vec, }; - #[test] - fn reachability() { - let mut root = Runtime::init_circuit(1, move |circuit| { - // Changes to the edges relation. - let mut edges = vec![ - zset! { Tup2(1, 2) => 1 }, - zset! { Tup2(2, 3) => 1}, - zset! { Tup2(1, 3) => 1}, - zset! { Tup2(3, 1) => 1}, - zset! { Tup2(3, 1) => -1}, - zset! { Tup2(1, 2) => -1}, - zset! { Tup2(2, 4) => 1, Tup2(4, 1) => 1 }, - zset! { Tup2(2, 3) => -1, Tup2(3, 2) => 1 }, - ] - .into_iter(); - - // Expected content of the reachability relation. - let mut outputs = vec![ - zset! { Tup2(1, 2) => 1 }, - zset! { Tup2(1, 2) => 1, Tup2(2, 3) => 1, Tup2(1, 3) => 1 }, - zset! { Tup2(1, 2) => 1, Tup2(2, 3) => 1, Tup2(1, 3) => 1 }, - zset! { Tup2(1, 1) => 1, Tup2(2, 2) => 1, Tup2(3, 3) => 1, Tup2(1, 2) => 1, Tup2(1, 3) => 1, Tup2(2, 3) => 1, Tup2(2, 1) => 1, Tup2(3, 1) => 1, Tup2(3, 2) => 1}, - zset! { Tup2(1, 2) => 1, Tup2(2, 3) => 1, Tup2(1, 3) => 1 }, - zset! { Tup2(2, 3) => 1, Tup2(1, 3) => 1 }, - zset! { Tup2(1, 3) => 1, Tup2(2, 3) => 1, Tup2(2, 4) => 1, Tup2(2, 1) => 1, Tup2(4, 1) => 1, Tup2(4, 3) => 1 }, - zset! { Tup2(1, 1) => 1, Tup2(2, 2) => 1, Tup2(3, 3) => 1, Tup2(4, 4) => 1, - Tup2(1, 2) => 1, Tup2(1, 3) => 1, Tup2(1, 4) => 1, - Tup2(2, 1) => 1, Tup2(2, 3) => 1, Tup2(2, 4) => 1, - Tup2(3, 1) => 1, Tup2(3, 2) => 1, Tup2(3, 4) => 1, - Tup2(4, 1) => 1, Tup2(4, 2) => 1, Tup2(4, 3) => 1 }, - ] - .into_iter(); - - let edges = circuit - .add_source(Generator::new(move || edges.next().unwrap())); - - let paths = circuit.recursive(|child, paths: Stream<_, OrdZSet>>| { - let edges = edges.delta0(child); - - let paths_indexed = paths.map_index(|&Tup2(x, y)| (y, x)); - let edges_indexed = edges.map_index(|Tup2(x, y)| (*x, *y)); - - Ok(edges.plus(&paths_indexed.join(&edges_indexed, |_via, from, to| Tup2(*from, *to)))) - }) - .unwrap(); - - paths.integrate().stream_distinct().inspect(move |ps| { - assert_eq!(*ps, outputs.next().unwrap()); - }); - Ok(()) - }) - .unwrap().0; - - for _ in 0..8 { - root.transaction().unwrap(); - } - } - // See https://github.com/feldera/feldera/issues/4168 #[test] fn issue4168() { @@ -355,15 +370,15 @@ mod test { } } - // Somewhat lame multiple recursion example to test RecursiveStreams impl for - // tuples: compute forward and backward reachability at the same time. - #[test] - fn reachability2() { - type Edges = Stream>>; + mod reachability { + use super::*; + use crate::FallbackZSet; + + type Edge = Tup2; - let mut root = Runtime::init_circuit(1, move |circuit| { - // Changes to the edges relation. - let mut edges = vec![ + /// Changes to the edges relation. + fn edges_data() -> Vec> { + vec![ zset! { Tup2(1, 2) => 1 }, zset! { Tup2(2, 3) => 1}, zset! { Tup2(1, 3) => 1}, @@ -373,59 +388,269 @@ mod test { zset! { Tup2(2, 4) => 1, Tup2(4, 1) => 1 }, zset! { Tup2(2, 3) => -1, Tup2(3, 2) => 1 }, ] - .into_iter(); + } - // Expected content of the reachability relation. - let output_vec = vec![ + /// Expected output to the reachable relation. + fn expected_reachable() -> Vec> { + vec![ zset! { Tup2(1, 2) => 1 }, zset! { Tup2(1, 2) => 1, Tup2(2, 3) => 1, Tup2(1, 3) => 1 }, zset! { Tup2(1, 2) => 1, Tup2(2, 3) => 1, Tup2(1, 3) => 1 }, - zset! { Tup2(1, 1) => 1, Tup2(2, 2) => 1, Tup2(3, 3) => 1, Tup2(1, 2) => 1, Tup2(1, 3) => 1, Tup2(2, 3) => 1, Tup2(2, 1) => 1, Tup2(3, 1) => 1, Tup2(3, 2) => 1}, + zset! { Tup2(1, 1) => 1, Tup2(2, 2) => 1, Tup2(3, 3) => 1, + Tup2(1, 2) => 1, Tup2(1, 3) => 1, Tup2(2, 3) => 1, + Tup2(2, 1) => 1, Tup2(3, 1) => 1, Tup2(3, 2) => 1}, zset! { Tup2(1, 2) => 1, Tup2(2, 3) => 1, Tup2(1, 3) => 1 }, zset! { Tup2(2, 3) => 1, Tup2(1, 3) => 1 }, - zset! { Tup2(1, 3) => 1, Tup2(2, 3) => 1, Tup2(2, 4) => 1, Tup2(2, 1) => 1, Tup2(4, 1) => 1, Tup2(4, 3) => 1 }, - zset! { Tup2(1, 1) => 1, Tup2(2, 2) => 1, Tup2(3, 3) => 1, Tup2(4, 4) => 1, - Tup2(1, 2) => 1, Tup2(1, 3) => 1, Tup2(1, 4) => 1, - Tup2(2, 1) => 1, Tup2(2, 3) => 1, Tup2(2, 4) => 1, - Tup2(3, 1) => 1, Tup2(3, 2) => 1, Tup2(3, 4) => 1, - Tup2(4, 1) => 1, Tup2(4, 2) => 1, Tup2(4, 3) => 1 }, - ]; - - let mut outputs = output_vec.clone().into_iter(); - let mut outputs2 = output_vec.into_iter(); - - let edges = circuit - .add_source(Generator::new(move || edges.next().unwrap())); - - let (paths, reverse_paths): (Stream<_, FallbackZSet>>, Stream<_, FallbackZSet>>) = - circuit.recursive(|child, (paths, reverse_paths): (Edges<_>, Edges<_>)| { - let edges = edges.delta0(child); - - let paths_indexed = paths.map_index(|&Tup2(x, y)| (y, x)); - let reverse_paths_indexed = reverse_paths.map_index(|&Tup2(x, y)| (y, x)); - let edges_indexed = edges.map_index(|Tup2(x,y)| (*x, *y)); - let reverse_edges = edges.map(|&Tup2(x, y)| Tup2(y, x)); - let reverse_edges_indexed = reverse_edges.map_index(|Tup2(x,y)| (*x, *y)); - - Ok((edges.plus(&paths_indexed.join(&edges_indexed, |_via, from, to| Tup2(*from, *to))), - reverse_edges.plus(&reverse_paths_indexed.join(&reverse_edges_indexed, |_via, from, to| Tup2(*from, *to))) - )) + zset! { Tup2(1, 3) => 1, Tup2(2, 3) => 1, Tup2(2, 4) => 1, + Tup2(2, 1) => 1, Tup2(4, 1) => 1, Tup2(4, 3) => 1 }, + zset! { Tup2(1, 1) => 1, Tup2(2, 2) => 1, Tup2(3, 3) => 1, + Tup2(4, 4) => 1, Tup2(1, 2) => 1, Tup2(1, 3) => 1, + Tup2(1, 4) => 1, Tup2(2, 1) => 1, Tup2(2, 3) => 1, + Tup2(2, 4) => 1, Tup2(3, 1) => 1, Tup2(3, 2) => 1, + Tup2(3, 4) => 1, Tup2(4, 1) => 1, Tup2(4, 2) => 1, + Tup2(4, 3) => 1 }, + ] + } + + #[test] + fn reachability() { + let edges_data = edges_data(); + let steps = edges_data.len(); + let mut edges = edges_data.into_iter(); + let mut expected_reachable = expected_reachable().into_iter(); + + let (mut handle, _) = Runtime::init_circuit(1, move |circuit| { + let edges = circuit.add_source(Generator::new(move || edges.next().unwrap())); + + let reachable = circuit + .recursive(|child, reachable: Stream<_, OrdZSet>| { + let edges = edges.delta0(child); + let edges_indexed = edges.map_index(|Tup2(x, y)| (*x, *y)); + + let reachable_indexed = reachable.map_index(|&Tup2(x, y)| (y, x)); + + let reachable_next = edges.plus( + &reachable_indexed + .join(&edges_indexed, |_via, from, to| Tup2(*from, *to)), + ); + + Ok(reachable_next) + }) + .unwrap(); + + reachable + .integrate() + .stream_distinct() + .inspect(move |reachable| { + assert_eq!(*reachable, expected_reachable.next().unwrap()); + }); + + Ok(()) }) .unwrap(); - paths.integrate().stream_distinct().inspect(move |ps| { - assert_eq!(*ps, outputs.next().unwrap()); - }); + for _ in 0..steps { + handle.transaction().unwrap(); + } + } - reverse_paths.map(|Tup2(x, y)| Tup2(*y, *x)).integrate().stream_distinct().inspect(move |ps: &OrdZSet<_>| { - assert_eq!(*ps, outputs2.next().unwrap()); - }); - Ok(()) - }) - .unwrap().0; + /// A rewrite of [`reachability()`] using + /// [`recursive_dynamic`](crate::ChildCircuit::recursive_dynamic): + /// A single recursive relation supplied as a one-element vector + /// (arity 1). It must produce exactly the same output as the + /// single-`Stream` implementation. + #[test] + fn reachability_dynamic() { + let edges_data = edges_data(); + let steps = edges_data.len(); + let mut edges = edges_data.into_iter(); + let mut expected_reachable = expected_reachable().into_iter(); + + let (mut handle, _) = Runtime::init_circuit(1, move |circuit| { + let edges = circuit.add_source(Generator::new(move || edges.next().unwrap())); + + let mut recursive_streams = circuit + .recursive_dynamic( + 1, + |child, mut recursive_streams: Vec>>| { + let edges = edges.delta0(child); + let edges_indexed = edges.map_index(|Tup2(x, y)| (*x, *y)); + + let reachable = &mut recursive_streams[0]; + let reachable_indexed = reachable.map_index(|&Tup2(x, y)| (y, x)); + + let reachable_next = edges.plus( + &reachable_indexed + .join(&edges_indexed, |_via, from, to| Tup2(*from, *to)), + ); + + // We can even reuse the allocated vector and spare us a reallocation. + *reachable = reachable_next; + Ok(recursive_streams) + }, + ) + .unwrap(); + + let reachable = recursive_streams.pop().unwrap(); + + reachable.integrate().stream_distinct().inspect(move |ps| { + assert_eq!(*ps, expected_reachable.next().unwrap()); + }); + + Ok(()) + }) + .unwrap(); - for _ in 0..8 { - root.transaction().unwrap(); + for _ in 0..steps { + handle.transaction().unwrap(); + } + } + + // Somewhat lame multiple recursion example to test RecursiveStreams impl for + // tuples: compute forward and backward reachability at the same time. + #[test] + fn reachability2() { + let edges_data = edges_data(); + let steps = edges_data.len(); + let mut edges = edges_data.into_iter(); + let expected_reachable = expected_reachable(); + let expected_reachable_reverse = expected_reachable.clone(); + let mut expected_reachable = expected_reachable.into_iter(); + let mut expected_reachable_reverse = expected_reachable_reverse.into_iter(); + + let (mut root, _) = Runtime::init_circuit(1, move |circuit| { + let edges = circuit.add_source(Generator::new(move || edges.next().unwrap())); + + let (reachable, reachable_reverse) = circuit + .recursive( + |child, + (reachable, reachable_reverse): ( + Stream<_, FallbackZSet>, + Stream<_, FallbackZSet>, + )| { + let edges = edges.delta0(child); + + let edges_indexed = edges.map_index(|Tup2(x, y)| (*x, *y)); + let reachable_indexed = reachable.map_index(|&Tup2(x, y)| (y, x)); + let reachable_reverse_indexed = + reachable_reverse.map_index(|&Tup2(x, y)| (y, x)); + let reverse_edges = edges.map(|&Tup2(x, y)| Tup2(y, x)); + let reverse_edges_indexed = + reverse_edges.map_index(|Tup2(x, y)| (*x, *y)); + + let reachable_next = edges.plus( + &reachable_indexed + .join(&edges_indexed, |_via, from, to| Tup2(*from, *to)), + ); + let reachable_reverse_next = reverse_edges.plus( + &reachable_reverse_indexed + .join(&reverse_edges_indexed, |_via, from, to| { + Tup2(*from, *to) + }), + ); + + Ok((reachable_next, reachable_reverse_next)) + }, + ) + .unwrap(); + + reachable.integrate().stream_distinct().inspect(move |ps| { + assert_eq!(*ps, expected_reachable.next().unwrap()); + }); + + reachable_reverse + .map(|Tup2(x, y)| Tup2(*y, *x)) + .integrate() + .stream_distinct() + .inspect(move |ps: &OrdZSet<_>| { + assert_eq!(*ps, expected_reachable_reverse.next().unwrap()); + }); + + Ok(()) + }) + .unwrap(); + + for _ in 0..steps { + root.transaction().unwrap(); + } + } + + /// A rewrite of [`reachability2()`] using + /// [`recursive_dynamic`](crate::ChildCircuit::recursive_dynamic): + /// Forward and backward reachability as two recursive relations + /// supplied as a two-element vector (arity 2). It must match the + /// tuple implementation. + #[test] + fn reachability2_dynamic() { + let edges_data = edges_data(); + let steps = edges_data.len(); + let mut edges = edges_data.into_iter(); + let expected_reachable = expected_reachable(); + let expected_reachable_reverse = expected_reachable.clone(); + let mut expected_reachable = expected_reachable.into_iter(); + let mut expected_reachable_reverse = expected_reachable_reverse.into_iter(); + + let (mut root, _) = Runtime::init_circuit(1, move |circuit| { + let edges = circuit.add_source(Generator::new(move || edges.next().unwrap())); + + let mut recursive_streams = circuit + .recursive_dynamic( + 2, + |child, mut recursive_streams: Vec>>| { + let edges = edges.delta0(child); + + let (reachable, rest) = recursive_streams.split_first_mut().unwrap(); + let reachable_reverse = rest.first_mut().unwrap(); + + let edges_indexed = edges.map_index(|Tup2(x, y)| (*x, *y)); + let reachable_indexed = reachable.map_index(|&Tup2(x, y)| (y, x)); + let reachable_reverse_indexed = + reachable_reverse.map_index(|&Tup2(x, y)| (y, x)); + let reverse_edges = edges.map(|&Tup2(x, y)| Tup2(y, x)); + let reverse_edges_indexed = + reverse_edges.map_index(|Tup2(x, y)| (*x, *y)); + + let reachable_next = edges.plus( + &reachable_indexed + .join(&edges_indexed, |_via, from, to| Tup2(*from, *to)), + ); + + let reachable_reverse_next = reverse_edges.plus( + &reachable_reverse_indexed + .join(&reverse_edges_indexed, |_via, from, to| { + Tup2(*from, *to) + }), + ); + + // We can even reuse the allocated vector and spare us a reallocation. + *reachable = reachable_next; + *reachable_reverse = reachable_reverse_next; + Ok(recursive_streams) + }, + ) + .unwrap(); + + let reachable_reverse = recursive_streams.pop().unwrap(); + let reachable = recursive_streams.pop().unwrap(); + + reachable.integrate().stream_distinct().inspect(move |ps| { + assert_eq!(*ps, expected_reachable.next().unwrap()); + }); + reachable_reverse + .map(|Tup2(x, y)| Tup2(*y, *x)) + .integrate() + .stream_distinct() + .inspect(move |ps: &OrdZSet<_>| { + assert_eq!(*ps, expected_reachable_reverse.next().unwrap()); + }); + + Ok(()) + }) + .unwrap(); + + for _ in 0..steps { + root.transaction().unwrap(); + } } } } diff --git a/crates/dbsp/src/operator/integral_weight_validator.rs b/crates/dbsp/src/operator/integral_weight_validator.rs new file mode 100644 index 00000000000..8000105d1a9 --- /dev/null +++ b/crates/dbsp/src/operator/integral_weight_validator.rs @@ -0,0 +1,149 @@ +//! Integral operator that panics when any record's accumulated weight +//! becomes negative. + +use crate::{ + ZWeight, + circuit::{Circuit, Stream, checkpointer::Checkpoint}, + trace::{BatchReader as TraceBatchReader, Cursor as TraceCursor}, + typed_batch::{BatchReader as TypedBatchReader, Spine, ZSet}, +}; + +impl Stream +where + C: Circuit, + B: Checkpoint + Clone + ZSet + 'static, + B::InnerBatch: Send, +{ + /// Integrates the stream, panicking if any record's weight in the integral + /// becomes negative. + /// + /// Maintains a running trace of the input stream. + /// After every clock tick, every key that appears in the + /// current delta is checked: if the key's weight in the trace is + /// negative, the operator panics, displaying the offending key, weight, + /// and a custom message. + /// + /// This operator is intended for debugging: it verifies that a stream + /// behaves like a valid set or multiset where every element has positive + /// multiplicity. + /// + /// The operator passes the input delta through unchanged as its output. + /// + /// # Panics + /// + /// Panics when any weight in the integral is < 0. + #[track_caller] + pub fn integral_weight_validator(&self, message: &str) -> Self { + let message = message.to_owned(); + self.circuit().region("integral_weight_validator", || { + let sharded = self.shard(); + let trace = sharded.integrate_trace(); + sharded.apply2(&trace, move |delta: &B, integrated: &Spine| { + let mut delta_cursor = TraceBatchReader::cursor(delta.inner()); + let mut trace_cursor = TraceBatchReader::cursor(integrated.inner()); + while delta_cursor.key_valid() { + // Only examine keys that appear in the current delta with a negative weight + let delta_weight: ZWeight = **delta_cursor.weight(); + if delta_weight < 0 { + let key = delta_cursor.key(); + if trace_cursor.seek_key_exact(key, None) { + debug_assert!(trace_cursor.val_valid()); + let weight: ZWeight = **trace_cursor.weight(); + if weight < 0 { + panic!("{message}: negative weight found {weight} for key {key:?}"); + } + } + } + delta_cursor.step_key(); + } + }); + self.clone() + }) + } +} + +#[cfg(test)] +mod tests { + use crate::{ + Circuit, Runtime, ZWeight, operator::Generator, typed_batch::OrdZSet, utils::Tup2, zset, + }; + + /// A stream of purely positive deltas should pass the check at every step. + #[test] + fn integral_weight_validator_positive_weights() { + let (mut circuit, ()) = Runtime::init_circuit(1, move |circuit| { + let mut step = 0u64; + let source = circuit.add_source(Generator::new(move || { + let batch = zset! { step => 1 }; + step += 1; + batch + })); + let out = source.integral_weight_validator("test_table"); + let mut expected_step = 0u64; + out.inspect(move |delta| { + assert_eq!(delta, &zset! { expected_step => 1 }); + expected_step += 1; + }); + Ok(()) + }) + .unwrap(); + + for _ in 0..10 { + circuit.transaction().unwrap(); + } + circuit.kill().unwrap(); + } + + /// Inserting an element once and then deleting it twice causes the trace + /// weight to go negative, which must trigger a panic. + #[test] + fn integral_weight_validator_panics_on_negative_integral() { + let (mut circuit, ()) = Runtime::init_circuit(1, move |circuit| { + let mut step = 0i64; + let source = circuit.add_source(Generator::new(move || { + // Step 0: insert key 42 with weight +1 → weight 1. + // Step 1: delete key 42 with weight -2 → weight -1 (panic). + let batch: OrdZSet = if step == 0 { + OrdZSet::from_tuples((), vec![Tup2(Tup2(42i64, ()), 1 as ZWeight)]) + } else { + OrdZSet::from_tuples((), vec![Tup2(Tup2(42i64, ()), -2 as ZWeight)]) + }; + step += 1; + batch + })); + source.integral_weight_validator("table 'test'"); + Ok(()) + }) + .unwrap(); + + circuit.transaction().unwrap(); // step 0: weight 1 (ok) + let err = circuit.transaction().expect_err("expected a panic"); + assert!( + err.to_string() + .contains("table 'test': negative weight found"), + "unexpected error: {err}" + ); + circuit.kill().unwrap(); + } + + /// A weight that goes negative (e.g., deleting something never inserted) + /// must also panic. + #[test] + fn integral_weight_validator_panics_on_negative_weight() { + let (mut circuit, ()) = Runtime::init_circuit(1, move |circuit| { + // Emit weight -1 immediately + let source = circuit.add_source(Generator::new(|| zset! { 42i64 => -1 })); + source.integral_weight_validator("table 'test'"); + Ok(()) + }) + .unwrap(); + + let err = circuit.transaction().expect_err("expected a panic"); + assert!( + err.to_string() + .contains("table 'test': negative weight found"), + "unexpected error: {err}" + ); + circuit.kill().unwrap(); + } +} diff --git a/crates/dbsp/src/operator/recursive.rs b/crates/dbsp/src/operator/recursive.rs index 52263a0963a..1538a738682 100644 --- a/crates/dbsp/src/operator/recursive.rs +++ b/crates/dbsp/src/operator/recursive.rs @@ -169,7 +169,7 @@ where /// zset_set! { Tup2(4, 5) }, /// // Remove an edge, breaking the cycle. /// zset! { Tup2(1, 2) => -1 }, - /// ] as [_; STEPS]) + /// ] as [OrdZSet>; STEPS]) /// .into_iter(); /// /// let edges = root_circuit @@ -182,7 +182,7 @@ where /// // Add a label to node 2. /// zset_set! { Tup2(2, "l2".to_string()) }, /// zset! { }, - /// ] as [_; STEPS]) + /// ] as [OrdZSet>; STEPS]) /// .into_iter(); /// /// let init_labels = root_circuit @@ -193,7 +193,7 @@ where /// zset! { Tup2(1, "l1".to_string()) => 1, Tup2(2, "l1".to_string()) => 1, Tup2(3, "l1".to_string()) => 1, Tup2(4, "l1".to_string()) => 1 }, /// zset! { Tup2(1, "l2".to_string()) => 1, Tup2(2, "l2".to_string()) => 1, Tup2(3, "l2".to_string()) => 1, Tup2(4, "l2".to_string()) => 1, Tup2(5, "l1".to_string()) => 1, Tup2(5, "l2".to_string()) => 1 }, /// zset! { Tup2(2, "l1".to_string()) => -1, Tup2(3, "l1".to_string()) => -1, Tup2(4, "l1".to_string()) => -1, Tup2(5, "l1".to_string()) => -1 }, - /// ] as [_; STEPS]) + /// ] as [OrdZSet>; STEPS]) /// .into_iter(); /// /// let labels = root_circuit.recursive(|child_circuit, labels: Stream<_, OrdZSet>>| { @@ -236,4 +236,191 @@ where }) .map(|streams| unsafe { S::typed_exports(&streams) }) } + + /// Like [`ChildCircuit::recursive`], but for a group of mutually recursive + /// streams whose size is only known at runtime. + /// + /// Whereas [`recursive`](ChildCircuit::recursive) fixes the number of + /// recursive streams at compile time (a single stream or a tuple of + /// streams), this method computes a fixed point over `arity` mutually + /// recursive streams that all share the same key type `K`, value type `V`, + /// and batch type `B`. The `arity` cannot be inferred, because the + /// recursive streams are the feedback Z-sets created *before* the closure + /// runs; it must therefore be supplied explicitly by the caller. + /// + /// The closure `f` receives a vector of `arity` recursive input streams and + /// must return a vector of exactly `arity` output streams, one per recursive + /// relation. + /// + /// Similar to [`recursive`](ChildCircuit::recursive), the underlying + /// circuit also applies an implicit distinct to the output of each + /// recursive step. + /// + /// # Panics + /// + /// Panics if the returned vector from the closure parameter has a different + /// length than the `arity` parameter. + /// + /// # Examples + /// + /// The circuit below computes a two-coloring (red and blue) of a graph. If + /// no node is both red and blue the graph happens to be bipartite. In the + /// first two computation steps the graph is bipartite but the added edge + /// in the third step adds an odd-length cycle which destroys the bipartite + /// property and all nodes are colored red and blue. + /// + /// ``` + /// use dbsp::{ + /// operator::Generator, + /// OrdZSet, Circuit, RootCircuit, Stream, zset, ZWeight, + /// utils::Tup2, Error as DbspError, Runtime, NestedCircuit + /// }; + /// + /// type Edge = Tup2; + /// type Node = usize; + /// + /// const STEPS: usize = 3; + /// + /// let mut init_data = ([ + /// vec![Tup2(0, 1)], + /// vec![], + /// vec![] + /// ] as [Vec>; STEPS]).into_iter(); + /// + /// let mut edges_data = ([ + /// // The first step adds a graph of four nodes: + /// // |0| --> |1| --> |2| --> |3| --> |4| + /// vec![ + /// Tup2(Tup2(0, 1), 1), + /// Tup2(Tup2(1, 2), 1), + /// Tup2(Tup2(2, 3), 1), + /// Tup2(Tup2(3, 4), 1), + /// ], + /// // Now, we have the following graph in total: + /// // |0| --> |1| --> |2| --> |3| --> |4| + /// // ^ | + /// // | | + /// // ------ |5| <----- + /// vec![Tup2(Tup2(2, 5), 1), Tup2(Tup2(5, 0), 1)], + /// // And we introduce an odd-length cycle, rendering the graph + /// // non-bipartite anymore (all nodes are red _and_ blue): + /// // |0| --> |1| --> |2| --> |3| --> |4| + /// // ^ | | + /// // | | | + /// // ------ |5| <----- | + /// // | | + /// // --------------------------------- + /// vec![Tup2(Tup2(4, 0), 1)], + /// ] as [Vec>; STEPS]).into_iter(); + /// + /// let mut expected_red_output = ([ + /// zset! { + /// 0 => 1, + /// 2 => 1, + /// 4 => 1, + /// }, + /// zset! {}, + /// zset! { + /// 1 => 1, + /// 3 => 1, + /// 5 => 1, + /// }, + /// ] as [OrdZSet; STEPS]).into_iter(); + /// + /// let mut expected_blue_output = ([ + /// zset! { + /// 1 => 1, + /// 3 => 1, + /// }, + /// zset! { + /// 5 => 1, + /// }, + /// zset! { + /// 0 => 1, + /// 2 => 1, + /// 4 => 1, + /// }, + /// ] as [OrdZSet; STEPS]).into_iter(); + /// + /// let (mut circuit_handle, ((init_input, edges_input), (red_output, blue_output))) = + /// Runtime::init_circuit(2, move |root_circuit| { + /// let (edges, edges_input) = root_circuit.add_input_zset::(); + /// let (init, init_input) = root_circuit.add_input_zset::(); + /// + /// let recursive_streams = root_circuit.recursive_dynamic( + /// 2, + /// |child_circuit, mut recursive_streams: Vec>>| { + /// // delta0 fires only at inner step 0, injecting the base case exactly once. + /// let edges = edges.delta0(child_circuit); + /// let init = init.delta0(child_circuit); + /// + /// let red = &recursive_streams[0]; + /// let blue = &recursive_streams[1]; + /// + /// let new_red = blue + /// .map_index(|blue_node| (*blue_node, *blue_node)) + /// .join( + /// &edges.map_index(|Tup2(from, to)| (*from, *to)), + /// |_blue_node, _, new_red_node| *new_red_node, + /// ) + /// .plus(&init); + /// + /// let new_blue = red.map_index(|red_node| (*red_node, *red_node)).join( + /// &edges.map_index(|Tup2(from, to)| (*from, *to)), + /// |_red_node, _, new_blue_node| *new_blue_node, + /// ); + /// + /// recursive_streams[0] = new_red; + /// recursive_streams[1] = new_blue; + /// Ok(recursive_streams) + /// }, + /// )?; + /// + /// let red_output = recursive_streams[0].accumulate_output(); + /// let blue_output = recursive_streams[1].accumulate_output(); + /// + /// Ok(( + /// (init_input, edges_input), + /// (red_output, blue_output), + /// )) + /// })?; + /// + /// for i in 0..STEPS { + /// init_input.append(&mut init_data.next().unwrap()); + /// edges_input.append(&mut edges_data.next().unwrap()); + /// circuit_handle.transaction().unwrap(); + /// assert_eq!(red_output.concat().consolidate(), expected_red_output.next().unwrap()); + /// assert_eq!(blue_output.concat().consolidate(), expected_blue_output.next().unwrap()); + /// } + /// + /// Ok::<(), DbspError>(()) + /// ``` + #[track_caller] + pub fn recursive_dynamic( + &self, + arity: usize, + f: F, + ) -> Result>>, SchedulerError> + where + B: Checkpoint + DynIndexedZSet + Send + Sync, + K: DBData + Erase, + V: DBData + Erase, + F: FnOnce( + &IterativeCircuit, + Vec, TypedBatch>>, + ) -> Result< + Vec, TypedBatch>>, + SchedulerError, + >, + { + let factories: Vec> = (0..arity) + .map(|_| DistinctFactories::new::()) + .collect(); + + self.dyn_recursive(&factories, |circuit, streams: Vec>| { + let typed = streams.iter().map(Stream::typed).collect(); + f(circuit, typed).map(|streams| streams.iter().map(Stream::inner).collect()) + }) + .map(|exports| exports.iter().map(Stream::typed).collect()) + } } diff --git a/crates/dbsp/src/tutorial.rs b/crates/dbsp/src/tutorial.rs index 2c4d5887693..a4caafd4f24 100644 --- a/crates/dbsp/src/tutorial.rs +++ b/crates/dbsp/src/tutorial.rs @@ -2056,9 +2056,9 @@ //! //! let (mut circuit_handle, output_handle) = Runtime::init_circuit(1, move |root_circuit| { //! let mut edges_data = ([ -//! zset_set! { Tup3(0_usize, 1_usize, 1_usize), Tup3(1, 2, 1), Tup3(2, 3, 2), Tup3(3, 4, 2) }, +//! zset_set! { Tup3(0, 1, 1), Tup3(1, 2, 1), Tup3(2, 3, 2), Tup3(3, 4, 2) }, //! zset! { Tup3(1, 2, 1) => -1 }, -//! ] as [_; STEPS]) +//! ] as [OrdZSet>; STEPS]) //! .into_iter(); //! //! let edges = root_circuit.add_source(Generator::new(move || edges_data.next().unwrap())); @@ -2119,7 +2119,7 @@ //! Tup4(1, 3, 3, 2) => -1, //! Tup4(1, 4, 5, 3) => -1, //! }, -//! ] as [_; STEPS]) +//! ] as [OrdZSet>; STEPS]) //! .into_iter(); //! //! closure.inspect(move |output| { @@ -2182,7 +2182,8 @@ //! # indexed_zset, //! # operator::{Generator, Min}, //! # utils::{Tup2, Tup3, Tup4}, -//! # zset_set, Circuit, NestedCircuit, OrdIndexedZSet, RootCircuit, Stream, IndexedZSetReader, Runtime +//! # zset_set, Circuit, NestedCircuit, OrdZSet, OrdIndexedZSet, RootCircuit, +//! # Stream, IndexedZSetReader, Runtime //! # }; //! # //! type Accumulator = @@ -2193,9 +2194,9 @@ //! # //! # let (mut circuit_handle, output_handle) = Runtime::init_circuit(1, move |root_circuit| { //! # let mut edges_data = ([ -//! # zset_set! { Tup3(0_usize, 1_usize, 1_usize), Tup3(1, 2, 1), Tup3(2, 3, 2), Tup3(3, 4, 2) }, +//! # zset_set! { Tup3(0, 1, 1), Tup3(1, 2, 1), Tup3(2, 3, 2), Tup3(3, 4, 2) }, //! # zset_set! { Tup3(4, 0, 3)} -//! # ] as [_; STEPS]) +//! # ] as [OrdZSet>; STEPS]) //! # .into_iter(); //! # //! # let edges = root_circuit.add_source(Generator::new(move || edges_data.next().unwrap())); diff --git a/crates/feldera-types/src/config.rs b/crates/feldera-types/src/config.rs index ad34a38e74d..e881be1693e 100644 --- a/crates/feldera-types/src/config.rs +++ b/crates/feldera-types/src/config.rs @@ -1723,6 +1723,33 @@ impl ConnectorConfig { a == b } + /// Compare two input connector configs modulo fields that only affect + /// runtime flow control and do not invalidate checkpointed connector state. + pub fn equal_for_input_checkpoint_replay(&self, other: &Self) -> bool { + let mut a = self.clone(); + let mut b = other.clone(); + a.normalize_for_input_checkpoint_replay(); + b.normalize_for_input_checkpoint_replay(); + a == b + } + + fn normalize_for_input_checkpoint_replay(&mut self) { + self.paused = false; + self.max_batch_size = None; + self.max_worker_batch_size = None; + self.max_queued_records = default_max_queued_records(); + self.max_queued_bytes = None; + } + + /// Adopt input connector settings that are safe to change while replaying + /// checkpointed connector state. + pub fn apply_input_checkpoint_replay_config_from(&mut self, other: &Self) { + self.max_batch_size = other.max_batch_size; + self.max_worker_batch_size = other.max_worker_batch_size; + self.max_queued_records = other.max_queued_records; + self.max_queued_bytes = other.max_queued_bytes; + } + /// Returns `max_queued_records` or, if it is not set, the default. pub fn max_queued_records(&self) -> u64 { self.max_queued_records diff --git a/crates/feldera-types/src/serde_with_context/serde_config.rs b/crates/feldera-types/src/serde_with_context/serde_config.rs index f0b265a4ea0..a9dbe279cf5 100644 --- a/crates/feldera-types/src/serde_with_context/serde_config.rs +++ b/crates/feldera-types/src/serde_with_context/serde_config.rs @@ -201,9 +201,9 @@ impl From for SqlSerdeConfig { JsonFlavor::DebeziumMySql => Self { time_format: TimeFormat::Micros, date_format: DateFormat::DaysSinceEpoch, - // Why is this missing fractions of second? - timestamp_format: TimestampFormat::String("%Y-%m-%dT%H:%M:%S%Z"), - timestamp_tz_format: TimestampFormat::String("%Y-%m-%dT%H:%M:%S%Z"), + // `2018-06-20T13:37:03Z` and `2018-06-20T13:37:03.123456Z`. + timestamp_format: TimestampFormat::String("%Y-%m-%dT%H:%M:%S%.f%Z"), + timestamp_tz_format: TimestampFormat::String("%Y-%m-%dT%H:%M:%S%.f%Z"), decimal_format: DecimalFormat::String, variant_format: VariantFormat::JsonString, binary_format: BinaryFormat::Array, diff --git a/crates/sqllib/src/timestamp.rs b/crates/sqllib/src/timestamp.rs index 2777bee5b78..6f9ed1917f2 100644 --- a/crates/sqllib/src/timestamp.rs +++ b/crates/sqllib/src/timestamp.rs @@ -2866,13 +2866,51 @@ some_polymorphic_function1!(extract_second, Date, Date, i64); some_polymorphic_function1!(extract_minute, Date, Date, i64); some_polymorphic_function1!(extract_hour, Date, Date, i64); +#[doc(hidden)] +pub fn datediff_hour_Date_Date(left: Date, right: Date) -> i32 { + (right.days() - left.days()) * 24 +} + +some_polymorphic_function2!(datediff_hour, Date, Date, Date, Date, i32); + #[doc(hidden)] pub fn datediff_day_Date_Date(left: Date, right: Date) -> i32 { - left.days() - right.days() + right.days() - left.days() } some_polymorphic_function2!(datediff_day, Date, Date, Date, Date, i32); +#[doc(hidden)] +pub fn datediff_week_Date_Date(left: Date, right: Date) -> i32 { + (right.days() - left.days()) / 7 +} + +some_polymorphic_function2!(datediff_week, Date, Date, Date, Date, i32); + +#[doc(hidden)] +pub fn datediff_month_Date_Date(left: Date, right: Date) -> i32 { + let interval = minus_LongInterval_Date_Date__(right, left); + interval.months() +} + +some_polymorphic_function2!(datediff_month, Date, Date, Date, Date, i32); + +#[doc(hidden)] +pub fn datediff_quarter_Date_Date(left: Date, right: Date) -> i32 { + datediff_year_Date_Date(left, right) * 4 + + (extract_quarter_Date(right) - extract_quarter_Date(left)) as i32 +} + +some_polymorphic_function2!(datediff_quarter, Date, Date, Date, Date, i32); + +#[doc(hidden)] +pub fn datediff_year_Date_Date(left: Date, right: Date) -> i32 { + let interval = minus_LongInterval_Date_Date__(right, left); + interval.years() +} + +some_polymorphic_function2!(datediff_year, Date, Date, Date, Date, i32); + #[doc(hidden)] pub fn format_date__(format: SqlString, date: Date) -> SqlString { SqlString::from(date.to_dateTime().format(format.str()).to_string()) @@ -4016,6 +4054,25 @@ mod test { ); } + /// Debezium MySQL emits `io.debezium.time.ZonedTimestamp` values with + /// fractional seconds when the column has sub-second precision. + /// The fraction must be preserved, not silently dropped. + #[test] + fn debezium_fractional_seconds() { + assert_eq!( + deserialize_with_debezium_config::( + r#"{"date": 16816, "time": 10800000000, "timestamp": "2018-06-20T13:37:03.123456Z", "timestampTz": "2018-06-20T13:37:03.123456Z" }"# + ) + .unwrap(), + TestStruct { + date: Date::from_days(16816), + time: Time::from_nanoseconds(10800000000000), + timestamp: Timestamp::from_microseconds(1529501823123456), + timestampTz: TimestampTz::from_microseconds(1529501823123456), + } + ); + } + #[test] fn snowflake_json() { assert_eq!( diff --git a/docs.feldera.com/docs/sql/grammar.md b/docs.feldera.com/docs/sql/grammar.md index d6277f3d538..b5b7c9e3a2a 100644 --- a/docs.feldera.com/docs/sql/grammar.md +++ b/docs.feldera.com/docs/sql/grammar.md @@ -806,6 +806,16 @@ FELDERA_USE_MULTI_JOINS = OFF`. Currently the following options are available: +`ENFORCE_POSITIVE_INPUTS` if set to `TRUE`, the compiler inserts a +runtime check after every input table that has no primary key. At +each step, the check verifies that no record in the accumulated +integral has a negative weight (i.e., the table has not received more +deletions for a key than insertions). A violation causes the pipeline +to panic. This option is intended for debugging pipelines that +produce unexpected results due to invalid input data. Note: this +setting is expensive, and will trigger a full backfill for the +affected tables. + `FELDERA_AVOID_STAR_JOINS` if set to true the compiler will not generate code using the new feature of "star joins", which are a simple form of multi-joins. We expect that option will be removed in diff --git a/js-packages/profiler-lib/src/profile.ts b/js-packages/profiler-lib/src/profile.ts index 506d9ff7bf1..f6bdd42ea68 100644 --- a/js-packages/profiler-lib/src/profile.ts +++ b/js-packages/profiler-lib/src/profile.ts @@ -998,16 +998,21 @@ export class Measurement { } case "merges": { let s = metric.value as MergesMetricValue; - let avg_step_time = TimeValue.fromDurationMetric(s.avg_step_time); + let avg_step_time = undefined; + let result = []; + if (s.avg_step_time !== undefined) { + avg_step_time = TimeValue.fromDurationMetric(s.avg_step_time); + result.push(new Measurement(metric_id + ".avg_step_time", Option.some(avg_step_time))); + } let batches = CountValue.fromCountMetric(s.batches); let merges = CountValue.fromCountMetric(s.merges); let steps = CountValue.fromCountMetric(s.steps); - return [ - new Measurement(metric_id + ".avg_step_time", Option.some(avg_step_time)), + result.push( new Measurement(metric_id + ".batches", Option.some(batches)), new Measurement(metric_id + ".merges", Option.some(merges)), new Measurement(metric_id + ".steps", Option.some(steps)), - ]; + ); + return result; } case "policy": { let s = metric.value as StringMetricValue; diff --git a/js-packages/web-console/src/lib/components/common/SlidingPanels.svelte b/js-packages/web-console/src/lib/components/common/SlidingPanels.svelte new file mode 100644 index 00000000000..9b0e0062b3c --- /dev/null +++ b/js-packages/web-console/src/lib/components/common/SlidingPanels.svelte @@ -0,0 +1,73 @@ + + + +
+ {#each pages as page (page.key)} + {#if page.key === current} +
+ {@render page.content()} +
+ {/if} + {/each} +
diff --git a/js-packages/web-console/src/lib/components/pipelines/Table.svelte b/js-packages/web-console/src/lib/components/pipelines/Table.svelte index 8ec51e0d53c..6240e633a96 100644 --- a/js-packages/web-console/src/lib/components/pipelines/Table.svelte +++ b/js-packages/web-console/src/lib/components/pipelines/Table.svelte @@ -7,6 +7,7 @@ import ThSort from '$lib/components/pipelines/table/ThSort.svelte' import { useElapsedTime } from '$lib/compositions/common/useElapsedTime' import { useLayoutSettings } from '$lib/compositions/layout/useLayoutSettings.svelte' + import { usePipelineManager } from '$lib/compositions/usePipelineManager.svelte' import { dateMax } from '$lib/functions/common/date' import { matchesSubstring } from '$lib/functions/common/string' import { type NamesInUnion, unionName } from '$lib/functions/common/union' @@ -17,6 +18,7 @@ } from '$lib/services/pipelineManager' import type { Snippet } from '$lib/types/svelte' import PipelineVersion from './table/PipelineVersion.svelte' + import Tags from './table/Tags.svelte' let { pipelines, @@ -83,6 +85,20 @@ pipelinesWithLastChange.filter((p) => matchesSubstring(p.name, nameSearch)) ) + // `knownTags` is the union of tags over every pipeline — the pool the per-row + // tag picker chooses from. + const knownTags = $derived.by(() => { + const tags = new Set() + for (const pipeline of pipelines) { + for (const tag of pipeline.tags) { + tags.add(tag) + } + } + return tags + }) + + const api = usePipelineManager() + const { formatElapsedTime } = useElapsedTime() const td = 'py-1 text-base border-t-[0.5px]' @@ -156,6 +172,14 @@ Message + Tags + + + Runtime + + - -
- {pipeline.connectors?.numErrors ?? '-'} -
- +
+ +
+ {pipeline.connectors?.numErrors ?? '-'} +
+
{formatElapsedTime(pipeline.lastStatusSince, 'dhm')} ago diff --git a/js-packages/web-console/src/lib/components/pipelines/Table.svelte.spec.ts b/js-packages/web-console/src/lib/components/pipelines/Table.svelte.spec.ts index 9982ac08175..eb390e67b5b 100644 --- a/js-packages/web-console/src/lib/components/pipelines/Table.svelte.spec.ts +++ b/js-packages/web-console/src/lib/components/pipelines/Table.svelte.spec.ts @@ -11,8 +11,8 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { page } from 'vitest/browser' import { render } from 'vitest-browser-svelte' -import type { PipelineThumb } from '$lib/services/pipelineManager' import { useLayoutSettings } from '$lib/compositions/layout/useLayoutSettings.svelte' +import type { PipelineThumb } from '$lib/services/pipelineManager' const SORT_KEY = 'layout/pipelines/table/sort' @@ -47,6 +47,7 @@ const thumb = (name: string): PipelineThumb => ({ name, description: '', + tags: [], status: 'Stopped', storageStatus: 'Cleared', deploymentStatusSince: lastChange[name], diff --git a/js-packages/web-console/src/lib/components/pipelines/editor/DownloadSupportBundle.svelte b/js-packages/web-console/src/lib/components/pipelines/editor/DownloadSupportBundle.svelte index 3d3defd7d10..913111d2d9c 100644 --- a/js-packages/web-console/src/lib/components/pipelines/editor/DownloadSupportBundle.svelte +++ b/js-packages/web-console/src/lib/components/pipelines/editor/DownloadSupportBundle.svelte @@ -1,7 +1,8 @@ + + + {#snippet trigger(toggle)} + {@const open = () => { + closeForm() + toggle() + }} +
+ {#each inlineTags as tag (tag)} + {@render chip(tag, open)} + {/each} + {#if overflowCount > 0} + + {tags.map(tagDisplayName).join(', ')} + {/if} + {#if tags.length === 0} + + {/if} +
+ {/snippet} + {#snippet content()} +
+ +
+ + {#snippet listPage()} +
+ +
+
+ {#each selectedTags as tag (tag)} + {@render tagRow(tag, true)} + {/each} + {#each unselectedTags as tag (tag)} + {@render tagRow(tag, false)} + {/each} +
+ + {/snippet} + + {#snippet createPage()} + {@render tagForm({ + title: 'Create a new tag', + submitLabel: candidateExists ? 'Assign' : 'Create', + submitDisabled: !newTagName.trim() || candidateError !== null, + lockedColor: createLockedColor, + validationError: candidateError, + onSubmit: submitCreate + })} + {/snippet} + + {#snippet editPage()} + {@render tagForm({ + title: 'Edit tag', + submitLabel: 'Save', + submitDisabled: !newTagName.trim() || candidateError !== null, + validationError: candidateError, + onSubmit: submitEdit, + onDelete: () => (page = 'delete') + })} + {/snippet} + + {#snippet deletePage()} +
+ + Delete tag +
+
+

+ The tag “{tagDisplayName(editingTag ?? '')}” will be removed from + {editingTagUsageCount} + {editingTagUsageCount === 1 ? 'pipeline' : 'pipelines'} it's currently assigned to. +
+ This cannot be undone. +

+
+ + +
+
+ {/snippet} + {/snippet} + + +{#snippet chip(tag: string, open: () => void)} + +{/snippet} + +{#snippet tagRow(tag: string, selected: boolean)} +
+ + +
+{/snippet} + +{#snippet tagForm(opts: { + title: string + submitLabel: string + submitDisabled: boolean + lockedColor?: string + validationError?: string | null + onSubmit: () => void + onDelete?: () => void +})} + {@const selectedColor = opts.lockedColor ?? newTagColor} + {@const colorName = tagColorPalette.find((c) => c.color === selectedColor)?.name ?? 'Custom'} +
+ + {opts.title} + {#if opts.onDelete} + + {/if} +
+
+ +
+ + Label color + {colorName} + +
+ {#each tagColorPalette as paletteColor (paletteColor.name)} + + {/each} +
+
+ + {#if opts.lockedColor !== undefined} +

+ The tag “{newTagName.trim()}” already exists. +

+ {/if} +
+{/snippet} diff --git a/js-packages/web-console/src/lib/components/pipelines/table/Tags.svelte.spec.ts b/js-packages/web-console/src/lib/components/pipelines/table/Tags.svelte.spec.ts new file mode 100644 index 00000000000..afe19b14ba4 --- /dev/null +++ b/js-packages/web-console/src/lib/components/pipelines/table/Tags.svelte.spec.ts @@ -0,0 +1,121 @@ +import { describe, expect, it, vi } from 'vitest' +import { page } from 'vitest/browser' +import { render } from 'vitest-browser-svelte' +import type { PipelineManagerApi } from '$lib/compositions/usePipelineManager.svelte' +import Tags from './Tags.svelte' + +// A stub Pipeline Manager client: only `patchPipeline` is exercised by the +// single-pipeline edit paths this component triggers. The cast keeps the test +// from having to fill in the dozens of unrelated client methods. +const makeApi = () => { + const patchPipeline = vi.fn().mockResolvedValue(undefined) + return { api: { patchPipeline } as unknown as PipelineManagerApi, patchPipeline } +} + +const renderTags = (props: { tags: string[]; knownTags?: string[] }) => { + const { api, patchPipeline } = makeApi() + const result = render(Tags, { + pipelineName: 'test-pipeline', + tags: props.tags, + knownTags: new Set(props.knownTags ?? props.tags), + api + }) + return { ...result, patchPipeline } +} + +describe('Tags.svelte', () => { + describe('chips', () => { + it('renders each assigned tag as a chip by display name, stripping the color', async () => { + renderTags({ tags: ['dev', 'prod|ef4444'] }) + await expect.element(page.getByRole('button', { name: 'dev' })).toBeVisible() + const prod = page.getByRole('button', { name: 'prod' }) + await expect.element(prod).toBeVisible() + // The chip's color dot uses the encoded color (#ef4444 -> rgb). + expect(prod.element().querySelector('span')).toHaveStyle({ + backgroundColor: 'rgb(239, 68, 68)' + }) + }) + + it('collapses the tags beyond the first two into a "+N" control', async () => { + renderTags({ tags: ['a', 'b', 'c', 'd'] }) + await expect + .element(page.getByRole('button', { name: 'Show all tags' })) + .toHaveTextContent('+2') + }) + + it('shows an add-tag affordance when the pipeline has no tags', async () => { + renderTags({ tags: [] }) + await expect.element(page.getByRole('button', { name: 'Tag' })).toBeVisible() + }) + }) + + describe('picker', () => { + it('lists assigned and unassigned tags, the assigned one checked', async () => { + renderTags({ tags: ['prod|ef4444'], knownTags: ['prod|ef4444', 'dev'] }) + await page.getByRole('button', { name: 'prod' }).click() + + // Both names appear in the list (selected first, then unselected). + await expect.element(page.getByText('dev')).toBeVisible() + const checkboxes = page.getByRole('checkbox').elements() + expect(checkboxes).toHaveLength(2) + const checked = checkboxes.filter((c) => (c as HTMLInputElement).checked) + expect(checked).toHaveLength(1) + }) + + it('assigning an unselected tag patches the pipeline with it added, kept sorted', async () => { + const { patchPipeline } = renderTags({ + tags: ['prod|ef4444'], + knownTags: ['prod|ef4444', 'dev'] + }) + await page.getByRole('button', { name: 'prod' }).click() + await page.getByRole('button', { name: 'dev' }).click() + + expect(patchPipeline).toHaveBeenCalledWith('test-pipeline', { + tags: ['dev', 'prod|ef4444'] + }) + }) + + it('unassigning an assigned tag patches the pipeline with it removed', async () => { + const { patchPipeline } = renderTags({ + tags: ['dev', 'prod|ef4444'], + knownTags: ['dev', 'prod|ef4444'] + }) + // Open via the overflow-free chip, then click the matching row to untoggle. + await page.getByRole('button', { name: 'dev' }).first().click() + // After opening, the same name exists as a chip and a row; the row is the + // second match. Click it to unassign. + await page.getByRole('button', { name: 'dev' }).last().click() + + expect(patchPipeline).toHaveBeenCalledWith('test-pipeline', { + tags: ['prod|ef4444'] + }) + }) + }) + + describe('create form validation', () => { + it('blocks an invalid name and surfaces the reason; a valid name is color-encoded', async () => { + const { patchPipeline } = renderTags({ tags: [] }) + await page.getByRole('button', { name: 'Tag' }).click() + await page.getByRole('button', { name: 'Create a new tag' }).click() + + const nameInput = page.getByPlaceholder('Tag name') + const createButton = page.getByRole('button', { name: 'Create' }) + + // A character outside the allowed set is rejected up front. + await nameInput.fill('a;b') + await expect.element(page.getByText(/may contain only/)).toBeVisible() + await expect.element(createButton).toBeDisabled() + expect(patchPipeline).not.toHaveBeenCalled() + + // A valid name enables submit; the default palette color is encoded as a + // "|rrggbb" suffix (first palette entry is red, #ef4444). + await nameInput.fill('qa') + await expect.element(createButton).toBeEnabled() + await createButton.click() + + expect(patchPipeline).toHaveBeenCalledWith('test-pipeline', { + tags: ['qa|ef4444'] + }) + }) + }) +}) diff --git a/js-packages/web-console/src/lib/functions/common/promise.spec.ts b/js-packages/web-console/src/lib/functions/common/promise.spec.ts new file mode 100644 index 00000000000..1c2938bc07a --- /dev/null +++ b/js-packages/web-console/src/lib/functions/common/promise.spec.ts @@ -0,0 +1,141 @@ +import { describe, expect, it, vi } from 'vitest' +import { promisePool } from './promise' + +/** A promise whose resolution the test controls explicitly. */ +const deferred = () => { + let resolve!: (value: T) => void + let reject!: (reason?: unknown) => void + const promise = new Promise((res, rej) => { + resolve = res + reject = rej + }) + return { promise, resolve, reject } +} + +/** + * Drain the microtask queue so resumed workers run to their next `await`. + * Resolving a promise and resuming its awaiter spans a few microtask turns, so + * yield several times rather than once. + */ +const flush = async () => { + for (let i = 0; i < 5; i++) { + await Promise.resolve() + } +} + +describe('promisePool', () => { + it('returns results in input order regardless of completion order', async () => { + const results = await promisePool([1, 2, 3, 4], 2, async (n) => n * 10) + expect(results).toEqual([10, 20, 30, 40]) + }) + + it('passes the item and its index to the worker', async () => { + const seen: [string, number][] = [] + await promisePool(['a', 'b', 'c'], 2, async (item, index) => { + seen.push([item, index]) + }) + expect(seen.sort((x, y) => x[1] - y[1])).toEqual([ + ['a', 0], + ['b', 1], + ['c', 2] + ]) + }) + + it('runs at most `concurrency` workers at once', async () => { + let inFlight = 0 + let peak = 0 + const gates = Array.from({ length: 6 }, () => deferred()) + const pool = promisePool(gates, 2, async (gate) => { + inFlight++ + peak = Math.max(peak, inFlight) + await gate.promise + inFlight-- + }) + // Release the gates one at a time, letting the pool refill its slots. + for (const gate of gates) { + await flush() + gate.resolve() + } + await pool + expect(peak).toBe(2) + }) + + it('slides the window: a slow item never stalls an idle slot', async () => { + const started: number[] = [] + const gates = [deferred(), deferred(), deferred(), deferred()] + const pool = promisePool([0, 1, 2, 3], 2, async (n) => { + started.push(n) + await gates[n].promise + }) + + // Slots 0 and 1 start immediately; 2 and 3 wait for a free slot. + await flush() + expect(started).toEqual([0, 1]) + + // Finish item 1 (the second slot); item 2 should take its place at once, + // even though item 0 is still running. + gates[1].resolve() + await flush() + expect(started).toEqual([0, 1, 2]) + + gates[0].resolve() + await flush() + expect(started).toEqual([0, 1, 2, 3]) + + gates[2].resolve() + gates[3].resolve() + await pool + }) + + it('handles an empty input without invoking the worker', async () => { + const fn = vi.fn(async (n: number) => n) + const results = await promisePool([], 4, fn) + expect(results).toEqual([]) + expect(fn).not.toHaveBeenCalled() + }) + + it('caps workers at the item count when concurrency exceeds it', async () => { + let inFlight = 0 + let peak = 0 + await promisePool([1, 2], 10, async (n) => { + inFlight++ + peak = Math.max(peak, inFlight) + await flush() + inFlight-- + return n + }) + expect(peak).toBe(2) + }) + + it('clamps non-positive concurrency to a single worker', async () => { + let inFlight = 0 + let peak = 0 + const results = await promisePool([1, 2, 3], 0, async (n) => { + inFlight++ + peak = Math.max(peak, inFlight) + await flush() + inFlight-- + return n + }) + expect(peak).toBe(1) + expect(results).toEqual([1, 2, 3]) + }) + + it('rejects when a worker rejects, and starts no further items', async () => { + const started: number[] = [] + await expect( + promisePool([0, 1, 2, 3, 4, 5], 2, async (n) => { + started.push(n) + if (n === 1) { + throw new Error('boom') + } + // Item 0 never settles on its own, so the only progress past the first + // two is whatever the failing worker would have pulled next — and it + // must pull nothing once the pool is rejecting. + await new Promise(() => {}) + }) + ).rejects.toThrow('boom') + // Only the initial two items ever started; no third item was claimed. + expect(started).toEqual([0, 1]) + }) +}) diff --git a/js-packages/web-console/src/lib/functions/common/promise.ts b/js-packages/web-console/src/lib/functions/common/promise.ts index b8f832547e3..4c04ad441ee 100644 --- a/js-packages/web-console/src/lib/functions/common/promise.ts +++ b/js-packages/web-console/src/lib/functions/common/promise.ts @@ -2,6 +2,46 @@ import { clearTimeout as clearWorkerTimeout, setTimeout as setWorkerTimeout } fr export const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)) +/** + * Map over `items` with a bounded number of concurrent workers — a sliding + * window rather than fixed batches. At most `concurrency` calls to `fn` are in + * flight at once; as each settles, the next item starts immediately. + * + * Results are returned in input order, like `Promise.all`. `fn` receives the + * item and its index. A rejection from `fn` rejects the whole pool (again like + * `Promise.all`), and no further items are started; callers that want to finish + * the rest should catch inside `fn`. + * + * The "workers" are not threads: everything runs on the single main JS thread, + * so this is concurrency, not parallelism. It only helps when `fn` spends its + * time awaiting (a network request, a timer) - those waits overlap. CPU-bound + * `fn`s gain nothing, since the thread still runs them one at a time. + * + * @param items Items to process. + * @param concurrency Maximum number of `fn` calls in flight at once (clamped to ≥ 1). + * @param fn Async worker invoked once per item. + * @returns Results in the same order as `items`. + */ +export const promisePool = async ( + items: readonly T[], + concurrency: number, + fn: (item: T, index: number) => Promise +): Promise => { + const results = new Array(items.length) + let next = 0 + // Each worker pulls the next unclaimed index until the list is drained; + // `next++` is atomic between awaits, so no two workers claim the same item. + const worker = async () => { + while (next < items.length) { + const index = next++ + results[index] = await fn(items[index], index) + } + } + const workerCount = Math.max(1, Math.min(concurrency, items.length)) + await Promise.all(Array.from({ length: workerCount }, worker)) + return results +} + const hasDocument = typeof document !== 'undefined' const isHidden = () => hasDocument && document.hidden const clearWorkerTimeoutSafely = (timeout: number) => { diff --git a/js-packages/web-console/src/lib/functions/pipelines/tags.spec.ts b/js-packages/web-console/src/lib/functions/pipelines/tags.spec.ts new file mode 100644 index 00000000000..f3e860f5046 --- /dev/null +++ b/js-packages/web-console/src/lib/functions/pipelines/tags.spec.ts @@ -0,0 +1,146 @@ +import { describe, expect, it } from 'vitest' +import { + defaultTagColor, + maximumTagLength, + parseTag, + tagColorOf, + tagColorPalette, + tagDisplayName, + tagWithColor, + validateTag +} from './tags' + +describe('parseTag', () => { + it('splits a name and its color', () => { + expect(parseTag('prod|ef4444')).toEqual({ name: 'prod', color: '#ef4444' }) + }) + + it('re-adds the "#" the stored form omits', () => { + expect(parseTag('x|22c55e').color).toBe('#22c55e') + }) + + it('rejects uppercase hex, keeping it as part of the name', () => { + expect(parseTag('prod|ABCDEF')).toEqual({ name: 'prod|ABCDEF' }) + expect(parseTag('prod|EF4444')).toEqual({ name: 'prod|EF4444' }) + }) + + it('returns no color for a bare name', () => { + expect(parseTag('prod')).toEqual({ name: 'prod' }) + }) + + it('keeps a "|" that is not followed by a six-digit hex color', () => { + expect(parseTag('a|b')).toEqual({ name: 'a|b' }) + expect(parseTag('env|staging')).toEqual({ name: 'env|staging' }) + expect(parseTag('prod|fff')).toEqual({ name: 'prod|fff' }) + expect(parseTag('prod|gggggg')).toEqual({ name: 'prod|gggggg' }) + }) + + it('treats only the final "|" segment as the color', () => { + expect(parseTag('a|b|ef4444')).toEqual({ name: 'a|b', color: '#ef4444' }) + }) +}) + +describe('tagDisplayName', () => { + it('strips a color suffix', () => { + expect(tagDisplayName('prod|ef4444')).toBe('prod') + expect(tagDisplayName('team billing|22c55e')).toBe('team billing') + }) + + it('returns a bare name unchanged', () => { + expect(tagDisplayName('prod')).toBe('prod') + expect(tagDisplayName('')).toBe('') + }) + + it('keeps a non-color "|" suffix', () => { + expect(tagDisplayName('a|b')).toBe('a|b') + }) +}) + +describe('tagColorOf', () => { + it('returns the encoded color as a CSS value', () => { + expect(tagColorOf('prod|ef4444')).toBe('#ef4444') + }) + + it('falls back to the default color when uncolored', () => { + expect(tagColorOf('prod')).toBe(defaultTagColor) + expect(tagColorOf('a|b')).toBe(defaultTagColor) + }) +}) + +describe('tagWithColor', () => { + it('encodes a color, dropping the leading "#"', () => { + expect(tagWithColor('prod', '#ef4444')).toBe('prod|ef4444') + }) + + it('replaces an existing suffix so re-coloring is idempotent', () => { + expect(tagWithColor('prod|ef4444', '#22c55e')).toBe('prod|22c55e') + expect(tagWithColor(tagWithColor('prod', '#ef4444'), '#ef4444')).toBe('prod|ef4444') + }) + + it('yields a bare name for a null or undefined color', () => { + expect(tagWithColor('prod|ef4444', null)).toBe('prod') + expect(tagWithColor('prod', undefined)).toBe('prod') + }) + + it('round-trips through parseTag', () => { + expect(parseTag(tagWithColor('prod', '#ef4444'))).toEqual({ name: 'prod', color: '#ef4444' }) + // A name that itself contains "|" survives the round-trip. + expect(parseTag(tagWithColor('a|b', '#ef4444'))).toEqual({ name: 'a|b', color: '#ef4444' }) + }) + + it('lowercases an uppercase CSS color so it stays recognizable', () => { + expect(tagWithColor('prod', '#ABCDEF')).toBe('prod|abcdef') + expect(parseTag(tagWithColor('prod', '#ABCDEF'))).toEqual({ name: 'prod', color: '#abcdef' }) + }) + + it('uses every palette color as a valid encoded tag', () => { + for (const { color } of tagColorPalette) { + const tag = tagWithColor('prod', color) + expect(validateTag(tag)).toBeNull() + expect(tagColorOf(tag)).toBe(color) + } + }) +}) + +describe('validateTag', () => { + it('accepts a plain name', () => { + expect(validateTag('prod')).toBeNull() + }) + + it('accepts the allowed punctuation', () => { + expect(validateTag('Mixed-1_2/3|4.5:6=7 8')).toBeNull() + }) + + it('accepts a fully encoded colored tag', () => { + expect(validateTag('prod|ef4444')).toBeNull() + }) + + it('accepts a tag exactly at the length limit', () => { + expect(validateTag('a'.repeat(maximumTagLength))).toBeNull() + }) + + it('rejects an empty tag', () => { + expect(validateTag('')).not.toBeNull() + }) + + it('rejects a tag over the length limit', () => { + expect(validateTag('a'.repeat(maximumTagLength + 1))).not.toBeNull() + }) + + it('rejects characters outside the allowed set', () => { + expect(validateTag('a;b')).not.toBeNull() + expect(validateTag('a#b')).not.toBeNull() + expect(validateTag('a,b')).not.toBeNull() + expect(validateTag('café')).not.toBeNull() + expect(validateTag('emoji😀')).not.toBeNull() + }) +}) + +describe('palette', () => { + it('is non-empty and every entry is a lowercase #rrggbb hex color', () => { + expect(tagColorPalette.length).toBeGreaterThan(0) + for (const { color } of tagColorPalette) { + expect(color).toMatch(/^#[0-9a-f]{6}$/) + } + }) +}) diff --git a/js-packages/web-console/src/lib/functions/pipelines/tags.ts b/js-packages/web-console/src/lib/functions/pipelines/tags.ts new file mode 100644 index 00000000000..ab5408750b2 --- /dev/null +++ b/js-packages/web-console/src/lib/functions/pipelines/tags.ts @@ -0,0 +1,103 @@ +/** + * Pipeline tag colors. + * + * The backend stores a tag as an opaque string, validates it against a fixed + * character set (`PATTERN_VALID_TAG` in the pipeline manager), and knows nothing + * about color. A tag's color is therefore encoded into the string itself as a + * suffix drawn only from that character set: the visible name, then a `|` + * separator, then a six-digit `rrggbb` hex color — no leading `#`, which the + * backend disallows. For example `"prod|ef4444"`. The suffix is optional; a bare + * `"prod"` is an uncolored tag, shown in a default color. + * + * A tag's *identity* is its name alone — the text before the color suffix — so + * `"prod|ef4444"` and `"prod|22c55e"` are the same tag in two colors; assigning + * one to a pipeline replaces the other. The suffix is recognized only when the + * text after the last `|` is exactly six hex digits, so a name that itself + * contains `|` (without a trailing color) is preserved intact. + */ + +/** The fixed palette of tag label colors (see design). */ +export const tagColorPalette = [ + { name: 'Red', color: '#ef4444' }, + { name: 'Purple', color: '#a855f7' }, + { name: 'Pink', color: '#ec4899' }, + { name: 'Orange', color: '#f97316' }, + { name: 'Yellow', color: '#eab308' }, + { name: 'Green', color: '#22c55e' }, + { name: 'Teal', color: '#14b8a6' }, + { name: 'Blue', color: '#3b82f6' }, + { name: 'Dark blue', color: '#1d4ed8' } +] as const + +export type TagColor = (typeof tagColorPalette)[number] + +/** The color shown for a tag that carries no color suffix. */ +export const defaultTagColor = '#9ca3af' + +/** + * The stored color suffix: a `|` followed by exactly six lowercase hex digits at + * the end of the string. No `#`, which the backend's tag character set disallows. + * Lowercase is the one canonical form (the palette below is lowercase); uppercase + * hex is not recognized as a color and stays part of the name. + */ +const storedColorSuffix = /\|([0-9a-f]{6})$/ + +/** + * Split a tag into its name and optional color. The suffix is recognized only + * when the text after the last `|` is exactly six hex digits, so a name that + * contains `|` but no trailing color is returned whole and uncolored. The + * returned color is a CSS `#rrggbb` value, with the `#` the stored form omits. + */ +export const parseTag = (tag: string): { name: string; color?: string } => { + const match = storedColorSuffix.exec(tag) + if (match) { + return { name: tag.slice(0, match.index), color: `#${match[1]}` } + } + return { name: tag } +} + +/** The visible text of a tag, with any color suffix removed. */ +export const tagDisplayName = (tag: string): string => parseTag(tag).name + +/** A tag's color as a CSS `#rrggbb` value, or {@link defaultTagColor} when none. */ +export const tagColorOf = (tag: string): string => parseTag(tag).color ?? defaultTagColor + +/** + * Encode `color` (a CSS `#rrggbb` value) into a tag as a `|rrggbb` suffix, + * dropping the leading `#` the backend disallows and replacing any existing + * suffix first so re-coloring is idempotent. The hex is lowercased so the result + * is recognized as a color even when given an uppercase CSS value. A null or + * undefined color yields the bare name. + */ +export const tagWithColor = (tag: string, color?: string | null): string => { + const name = tagDisplayName(tag) + return color ? `${name}|${color.replace(/^#/, '').toLowerCase()}` : name +} + +/** Maximum length of a single stored tag, mirroring the backend. */ +export const maximumTagLength = 50 + +/** + * Characters a tag may contain, mirroring the backend's `PATTERN_VALID_TAG` + * (crates/pipeline-manager/src/db/types/utils.rs). + * The color suffix is a subset of this character set. + */ +export const tagPattern = /^[a-zA-Z0-9 ._/|\\:=-]+$/ + +/** + * Check a fully encoded tag against the same rules the backend enforces, so the + * UI can reject a bad tag up front instead of waiting for the API to fail. + * Returns a human-readable reason, or null when the tag is valid. + */ +export const validateTag = (tag: string): string | null => { + if (tag.length === 0) { + return 'A tag cannot be empty.' + } + if (tag.length > maximumTagLength) { + return `A tag may be at most ${maximumTagLength} characters; this one is ${tag.length}.` + } + if (!tagPattern.test(tag)) { + return 'A tag may contain only letters, numbers, spaces, and the characters . _ / | \\ : = -' + } + return null +} diff --git a/js-packages/web-console/src/lib/services/pipelineManager.ts b/js-packages/web-console/src/lib/services/pipelineManager.ts index 2e9683366e5..790ff8f6175 100644 --- a/js-packages/web-console/src/lib/services/pipelineManager.ts +++ b/js-packages/web-console/src/lib/services/pipelineManager.ts @@ -36,6 +36,7 @@ import { listClusterEvents, listPipelineEvents, listPipelines, + type PatchPipeline, type PipelineSelectedInfo, type PostPutPipeline, type ProgramError, diff --git a/openapi.json b/openapi.json index af9d5c942b5..6e10540482e 100644 --- a/openapi.json +++ b/openapi.json @@ -10,7 +10,7 @@ "license": { "name": "MIT OR Apache-2.0" }, - "version": "0.316.0" + "version": "0.317.0" }, "paths": { "/config/authentication": { diff --git a/python/dbt-feldera/pyproject.toml b/python/dbt-feldera/pyproject.toml index 5169c7ca265..d5b063af7d5 100644 --- a/python/dbt-feldera/pyproject.toml +++ b/python/dbt-feldera/pyproject.toml @@ -6,7 +6,7 @@ build-backend = "setuptools.build_meta" name = "dbt-feldera" readme = "README.md" description = "The dbt adapter for Feldera — DBSP-native incremental view maintenance" -version = "0.316.0" +version = "0.317.0" license = "MIT" requires-python = ">=3.10" authors = [ diff --git a/python/feldera/pipeline.py b/python/feldera/pipeline.py index e6bf400c6a1..adb462b2cc6 100644 --- a/python/feldera/pipeline.py +++ b/python/feldera/pipeline.py @@ -1468,6 +1468,10 @@ def description(self) -> str: def tags(self) -> List[str]: """ Return the tags of the pipeline. + + Tags are returned verbatim, including the ``|rrggbb`` color suffix the web + console uses to encode a tag's color. Use :func:`feldera.tags.tag_display_name` + to obtain the tag's text label. """ self.refresh(PipelineFieldSelector.STATUS) diff --git a/python/feldera/rest/feldera_client.py b/python/feldera/rest/feldera_client.py index 5417dfffc04..9fd7d7e2a2d 100644 --- a/python/feldera/rest/feldera_client.py +++ b/python/feldera/rest/feldera_client.py @@ -18,6 +18,7 @@ from feldera.rest.errors import FelderaAPIError, FelderaTimeoutError from feldera.rest.feldera_config import FelderaConfig from feldera.rest.pipeline import Pipeline +from feldera.tags import _normalize_tags from feldera.rest.retry import RetryConfig logger = logging.getLogger(__name__) @@ -332,7 +333,7 @@ def create_pipeline(self, pipeline: Pipeline, wait: bool = True) -> Pipeline: "program_config": pipeline.program_config, "runtime_config": pipeline.runtime_config, "description": pipeline.description or "", - "tags": pipeline.tags, + "tags": _normalize_tags(pipeline.tags), } self.http.post( @@ -365,7 +366,7 @@ def create_or_update_pipeline( "program_config": pipeline.program_config, "runtime_config": pipeline.runtime_config, "description": pipeline.description or "", - "tags": pipeline.tags, + "tags": _normalize_tags(pipeline.tags), } self.http.put( @@ -398,6 +399,10 @@ def patch_pipeline( stored one, and a field left as ``None`` is omitted from the request and leaves the stored value untouched. To clear the description or tags, pass an empty string or empty list, respectively. + + Tags, when provided, are normalized before being sent: color variants of + the same display name are collapsed and the result is stored in API server database + in lexicographic order (see :mod:`feldera.tags`). """ self.http.patch( @@ -409,7 +414,7 @@ def patch_pipeline( "program_config": program_config, "runtime_config": runtime_config, "description": description, - "tags": tags, + "tags": _normalize_tags(tags) if tags is not None else None, }, ) diff --git a/python/feldera/tags.py b/python/feldera/tags.py new file mode 100644 index 00000000000..33b62ec478c --- /dev/null +++ b/python/feldera/tags.py @@ -0,0 +1,51 @@ +""" +Pipeline tag conventions. + +The backend stores a tag as an opaque string and validates it against an allowed +character set. The web console encodes a tag's color into the string itself +as a suffix that character set permits: the visible name, then a ``|`` separator, +then a six-digit ``rrggbb`` hex color -- e.g. ``"prod|ef4444"``. The suffix +is optional, so a bare ``"prod"`` is an uncolored tag; ``"prod|ef4444"`` and +``"prod|22c55e"`` are considered a tag collision which can but should not occur. + +To stay consistent with the web console, the SDK treats a tag's *display name* -- +its name with any color suffix removed -- as the tag's identity, and enforces two +invariants whenever a pipeline's tags are written: + +* **Variant exclusivity.** A pipeline carries at most one tag per display name. + When a list holds several color variants of the same name, the last one wins. +* **Lexicographic order.** Tags are sorted lexicographically by the SDK + before sending to the API server. +""" + +import re +from typing import Iterable, List + +# A tag's color suffix: a ``|`` followed by exactly six lowercase hex digits at +# the end of the string (no ``#``, which the backend's tag character set +# disallows). Lowercase is the one canonical form; uppercase hex is therefore +# not recognized as a color and stays part of the name. +# Recognized only in this exact form, so a name that itself contains +# ``|`` (without a trailing color) keeps its full text. +_COLOR_SUFFIX = re.compile(r"\|[0-9a-f]{6}$") + + +def tag_display_name(tag: str) -> str: + """Return a tag's visible text: its name with any color suffix removed.""" + + return _COLOR_SUFFIX.sub("", tag) + + +def _normalize_tags(tags: Iterable[str]) -> List[str]: + """ + Collapse color variants of the same tag and sort the result. + + Tags that share a display name are deduplicated, keeping the last occurrence, + and the survivors are returned in lexicographic order. The color suffix that + encodes a tag's color is preserved. + """ + + by_display_name: dict[str, str] = {} + for tag in tags: + by_display_name[tag_display_name(tag)] = tag + return sorted(by_display_name.values()) diff --git a/python/felderize/pyproject.toml b/python/felderize/pyproject.toml index 1ea315039a7..d5605195057 100644 --- a/python/felderize/pyproject.toml +++ b/python/felderize/pyproject.toml @@ -6,7 +6,7 @@ build-backend = "setuptools.build_meta" name = "felderize" readme = "README.md" description = "SQL dialect to Feldera SQL translator agent" -version = "0.316.0" +version = "0.317.0" license = "MIT" requires-python = ">=3.10" authors = [ diff --git a/python/pyproject.toml b/python/pyproject.toml index 6552d742e3d..e163b585257 100644 --- a/python/pyproject.toml +++ b/python/pyproject.toml @@ -6,7 +6,7 @@ build-backend = "setuptools.build_meta" name = "feldera" readme = "README.md" description = "The feldera python client" -version = "0.316.0" +version = "0.317.0" license = "MIT" requires-python = ">=3.10,<3.14" authors = [ diff --git a/python/tests/platform/test_bootstrapping.py b/python/tests/platform/test_bootstrapping.py index 56f49366b29..325e7dad9e9 100644 --- a/python/tests/platform/test_bootstrapping.py +++ b/python/tests/platform/test_bootstrapping.py @@ -702,3 +702,66 @@ def gen_sql(connectors): pipeline.checkpoint(True) pipeline.stop(force=True) + + +@enterprise_only +@single_host_only +@gen_pipeline_name +def test_bootstrap_input_flow_control_changes(pipeline_name): + """ + Enterprise: input connector flow-control changes should not require bootstrapping. + """ + + def gen_sql(max_queued_records): + connectors = json.dumps( + [ + { + "name": "datagen_connector", + "transport": { + "name": "datagen", + "config": {"plan": [{"limit": 10}]}, + }, + "max_queued_records": max_queued_records, + "max_queued_bytes": max_queued_records * 100, + "max_batch_size": max_queued_records + 1, + "max_worker_batch_size": max_queued_records + 2, + } + ] + ) + return f"""CREATE TABLE t1(x int) +with ( + 'materialized' = 'true', + 'connectors' = '{connectors}' +); +CREATE MATERIALIZED VIEW v1 AS SELECT COUNT(*) AS c FROM t1; +""" + + pipeline = PipelineBuilder( + TEST_CLIENT, + pipeline_name, + sql=gen_sql(100), + runtime_config=RuntimeConfig( + workers=FELDERA_TEST_NUM_WORKERS, + hosts=FELDERA_TEST_NUM_HOSTS, + fault_tolerance_model=None, + ), + ).create_or_replace() + + pipeline.start() + assert pipeline.status() == PipelineStatus.RUNNING + + pipeline.wait_for_completion(timeout_s=300) + result = list(pipeline.query("SELECT * FROM v1;")) + assert result == [{"c": 10}] + + pipeline.checkpoint(True) + pipeline.stop(force=True) + + pipeline.modify(sql=gen_sql(200)) + pipeline.start(bootstrap_policy=BootstrapPolicy.REJECT, timeout_s=300) + assert pipeline.status() == PipelineStatus.RUNNING + + result = list(pipeline.query("SELECT * FROM v1;")) + assert result == [{"c": 10}] + + pipeline.stop(force=True) diff --git a/python/tests/platform/test_pipeline_builder.py b/python/tests/platform/test_pipeline_builder.py index ce4bb70e10a..08c09895875 100644 --- a/python/tests/platform/test_pipeline_builder.py +++ b/python/tests/platform/test_pipeline_builder.py @@ -101,6 +101,13 @@ def test_tags(self): pipeline.modify(tags=[]) assert pipeline.tags() == [] + # The SDK normalizes tags before storing them, matching the web console: + # tags are saved in sorted order, and color variants of the same name + # (the same text with different "|rrggbb" color suffixes) are collapsed to + # the last one supplied. The surviving variant keeps its color. + pipeline.modify(tags=["prod", "alpha", "prod|ef4444"]) + assert pipeline.tags() == ["alpha", "prod|ef4444"] + if __name__ == "__main__": unittest.main() diff --git a/python/tests/platform/test_output_buffer_size_limit.py b/python/tests/runtime/test_output_buffer_size_limit.py similarity index 77% rename from python/tests/platform/test_output_buffer_size_limit.py rename to python/tests/runtime/test_output_buffer_size_limit.py index 8b846d3df22..55b80ba882e 100644 --- a/python/tests/platform/test_output_buffer_size_limit.py +++ b/python/tests/runtime/test_output_buffer_size_limit.py @@ -13,7 +13,7 @@ This test enables output buffering on a Delta Lake sink without setting a time limit, feeds it more than 10,000,000 records, and verifies that records are -written out: the pipeline's completed-record count advances past the default +written out: the sink's transmitted-record count advances past the default size cap. """ @@ -25,8 +25,6 @@ from tests import TEST_CLIENT from tests.utils import DeltaTestLocation, wait_for_condition -from .helper import gen_pipeline_name - # Default value of ``max_output_buffer_size_records``. The buffer must flush # once it grows past this many records, even without a time limit. _DEFAULT_MAX_OUTPUT_BUFFER_SIZE_RECORDS = 10_000_000 @@ -36,7 +34,6 @@ _NUM_RECORDS = 12_000_000 -@gen_pipeline_name def test_output_buffer_flushes_at_default_size_limit(pipeline_name): """A buffered Delta sink with no time limit still flushes at 10M records.""" @@ -98,18 +95,22 @@ def test_output_buffer_flushes_at_default_size_limit(pipeline_name): pipeline.start() - try: - # The buffer flushes once it crosses the 10M default size cap, pushing - # those records through the Delta sink and advancing the completed - # count past the cap. - wait_for_condition( - "completed-record count advances past the default buffer size cap", - lambda: ( - (pipeline.stats().global_metrics.total_completed_records or 0) - >= _DEFAULT_MAX_OUTPUT_BUFFER_SIZE_RECORDS - ), - timeout_s=600.0, - poll_interval_s=2.0, - ) - finally: - pipeline.stop(force=True) + # The buffer flushes once it grows past the 10M default size cap, pushing + # its contents through the Delta sink. Poll the sink's transmitted-record + # count, which the endpoint updates after the Delta commit succeeds. + # + # Do not poll total_completed_records here: a flush triggered by a step + # whose transaction commit is still in progress credits only the + # previous commit boundary; the remaining + # tail never flushes because the default time cap is infinite, so + # the completed count would stay below the cap forever. + wait_for_condition( + "Delta sink transmits at least the default buffer size cap", + lambda: any( + (output.metrics.transmitted_records or 0) + >= _DEFAULT_MAX_OUTPUT_BUFFER_SIZE_RECORDS + for output in pipeline.stats().outputs + ), + timeout_s=600.0, + poll_interval_s=2.0, + ) diff --git a/python/tests/unit/test_tags.py b/python/tests/unit/test_tags.py new file mode 100644 index 00000000000..5330c1aec88 --- /dev/null +++ b/python/tests/unit/test_tags.py @@ -0,0 +1,67 @@ +"""Unit tests for the pipeline tag conventions in :mod:`feldera.tags`.""" + +from feldera.tags import _normalize_tags, tag_display_name + + +def test_display_name_strips_color_suffix(): + # A trailing "|rrggbb" suffix is removed; everything else is the visible name. + assert tag_display_name("prod") == "prod" + assert tag_display_name("prod|ef4444") == "prod" + assert tag_display_name("team billing|22c55e") == "team billing" + # Only lowercase hex is a color; uppercase hex stays part of the name. + assert tag_display_name("prod|ef4444") == "prod" + assert tag_display_name("prod|ABCDEF") == "prod|ABCDEF" + assert tag_display_name("prod|EF4444") == "prod|EF4444" + # A name containing "|" but no valid trailing color is returned whole. + assert tag_display_name("a|b") == "a|b" + assert tag_display_name("env|staging") == "env|staging" + # Wrong-length or malformed suffixes are not colors. + assert tag_display_name("prod|fff") == "prod|fff" + assert tag_display_name("prod|gggggg") == "prod|gggggg" + # Only the final "|" segment is the color; an inner "|" stays in the name. + assert tag_display_name("a|b|ef4444") == "a|b" + assert tag_display_name("") == "" + + +def test_normalize_sorts_lexicographically(): + assert _normalize_tags(["prod", "dev", "staging"]) == ["dev", "prod", "staging"] + + +def test_normalize_is_idempotent(): + once = _normalize_tags(["beta", "alpha", "alpha|ef4444"]) + assert _normalize_tags(once) == once + + +def test_normalize_empty(): + assert _normalize_tags([]) == [] + + +def test_normalize_collapses_color_variants_keeping_last(): + # "prod" and "prod|ef4444" share a display name, so only one survives, and it + # is the last one supplied -- mirroring the console, where assigning a tag + # drops the other color variants of the same name. + assert _normalize_tags(["prod", "prod|ef4444"]) == ["prod|ef4444"] + assert _normalize_tags(["prod|ef4444", "prod"]) == ["prod"] + + +def test_normalize_preserves_color_of_survivor(): + # The surviving variant keeps its color suffix, so its color is not lost. + result = _normalize_tags(["dev", "prod|ef4444", "qa"]) + assert result == ["dev", "prod|ef4444", "qa"] + + +def test_normalize_distinct_names_with_colors_all_kept(): + # Different display names never collide, regardless of color. + assert _normalize_tags(["b|ef4444", "a|22c55e", "c"]) == [ + "a|22c55e", + "b|ef4444", + "c", + ] + + +def test_normalize_accepts_any_iterable(): + assert _normalize_tags(iter(["b", "a"])) == ["a", "b"] + assert _normalize_tags(("staging", "prod", "staging|22c55e")) == [ + "prod", + "staging|22c55e", + ] diff --git a/python/uv.lock b/python/uv.lock index 1e78bddb329..081ef9c8bdc 100644 --- a/python/uv.lock +++ b/python/uv.lock @@ -9,7 +9,7 @@ resolution-markers = [ ] [options] -exclude-newer = "2026-06-21T02:26:59.597343031Z" +exclude-newer = "2026-06-26T04:07:02.16602316Z" exclude-newer-span = "P1W" [[package]] @@ -273,7 +273,7 @@ wheels = [ [[package]] name = "feldera" -version = "0.316.0" +version = "0.317.0" source = { editable = "." } dependencies = [ { name = "numpy", version = "2.2.6", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version < '3.11'" }, diff --git a/sql-to-dbsp-compiler/SQL-compiler/pom.xml b/sql-to-dbsp-compiler/SQL-compiler/pom.xml index 60d5d011f50..831aa57521f 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/pom.xml +++ b/sql-to-dbsp-compiler/SQL-compiler/pom.xml @@ -19,7 +19,7 @@ 1.43.0 2.22.0 3.1.12 - 0.316.0 + 0.317.0 1.28.0 diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/codegen/includes/parserImpls.ftl b/sql-to-dbsp-compiler/SQL-compiler/src/main/codegen/includes/parserImpls.ftl index e24962d33f0..90ab6f19830 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/main/codegen/includes/parserImpls.ftl +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/codegen/includes/parserImpls.ftl @@ -36,7 +36,7 @@ SqlNode DateaddFunctionCall() : { ( { op = SqlLibraryOperators.DATE_PART; } | { op = SqlLibraryOperators.DATEADD; } - | { op = SqlStdOperatorTable.TIMESTAMP_DIFF; } + | { op = SqlLibraryOperators.DATEDIFF; } | { op = SqlLibraryOperators.DATEPART; } ) { s = span(); } diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/CompilerMain.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/CompilerMain.java index 8f67060514c..c49ef2abc89 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/CompilerMain.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/CompilerMain.java @@ -175,7 +175,7 @@ CompilerMessages run() { return compiler.messages; // The following runs all compilation stages DBSPCircuit circuit = compiler.getFinalCircuit(false); - if (options.ioOptions.anonymize) + if (options.ioOptions.anonymize || options.ioOptions.format) // The front-end has already emitted the output, we are done. return compiler.messages; if (compiler.hasErrors()) diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/circuit/operator/DBSPWeightValidatorOperator.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/circuit/operator/DBSPWeightValidatorOperator.java new file mode 100644 index 00000000000..3c28439bbfb --- /dev/null +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/circuit/operator/DBSPWeightValidatorOperator.java @@ -0,0 +1,55 @@ +package org.dbsp.sqlCompiler.circuit.operator; + +import com.fasterxml.jackson.databind.JsonNode; +import org.dbsp.sqlCompiler.circuit.OutputPort; +import org.dbsp.sqlCompiler.compiler.backend.JsonDecoder; +import org.dbsp.sqlCompiler.compiler.frontend.calciteObject.CalciteEmptyRel; +import org.dbsp.sqlCompiler.compiler.frontend.calciteObject.CalciteRelNode; +import org.dbsp.sqlCompiler.compiler.visitors.VisitDecision; +import org.dbsp.sqlCompiler.compiler.visitors.outer.CircuitVisitor; +import org.dbsp.sqlCompiler.ir.expression.DBSPExpression; +import org.dbsp.sqlCompiler.ir.type.DBSPType; +import org.dbsp.util.Utilities; + +import javax.annotation.Nullable; +import java.util.List; + +/** Operator that panics at runtime if the running integral of its input stream + * contains any record with a non-positive weight. */ +public final class DBSPWeightValidatorOperator extends DBSPUnaryOperator { + /** Panic message. */ + public final String message; + + public DBSPWeightValidatorOperator(CalciteRelNode node, OutputPort source, String message) { + super(node, "integral_weight_validator", null, source.outputType(), source.isMultiset(), source); + this.message = message; + } + + @Override + public void accept(CircuitVisitor visitor) { + visitor.push(this); + VisitDecision decision = visitor.preorder(this); + if (!decision.stop()) + visitor.postorder(this); + visitor.pop(this); + } + + @Override + public DBSPSimpleOperator with( + @Nullable DBSPExpression function, DBSPType outputType, + List newInputs, boolean force) { + if (this.mustReplace(force, function, newInputs, outputType)) { + return new DBSPWeightValidatorOperator( + this.getRelNode(), newInputs.get(0), this.message).copyAnnotations(this); + } + return this; + } + + @SuppressWarnings("unused") + public static DBSPWeightValidatorOperator fromJson(JsonNode node, JsonDecoder decoder) { + CommonInfo info = commonInfoFromJson(node, decoder); + String message = Utilities.getStringProperty(node, "message"); + return new DBSPWeightValidatorOperator(CalciteEmptyRel.INSTANCE, info.getInput(0), message) + .addAnnotations(info.annotations(), DBSPWeightValidatorOperator.class); + } +} diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/CompilerOptions.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/CompilerOptions.java index 55cbe5246e8..59c1c4f3d97 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/CompilerOptions.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/CompilerOptions.java @@ -159,6 +159,9 @@ public String diff(CompilerOptions other) { public static class IO implements IDiff, IValidate { @Parameter(names = "--anonymize", description = "Produce in the output file an anonymized version of the input program") public boolean anonymize = false; + @Parameter(names = "--format", + description = "Output the SQL program reformatted") + public boolean format = false; @DynamicParameter(names = "-T", description = "Specify logging level for a class (can be repeated)") public Map loggingLevel = new HashMap<>(); @@ -278,6 +281,7 @@ public String toString() { ",\n\tnoRust=" + this.noRust + ",\n\toutputFile=" + Utilities.singleQuote(this.outputFile) + ",\n\tquiet=" + this.quiet + + ",\n\tformat=" + this.format + ",\n\truntime=" + Utilities.singleQuote(this.runtimePath) + ",\n\ttrimInputs=" + this.trimInputs + ",\n\tverbosity=" + this.verbosity + diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/DBSPCompiler.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/DBSPCompiler.java index 6d9bd146b2d..1a24181368f 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/DBSPCompiler.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/DBSPCompiler.java @@ -39,6 +39,9 @@ import org.apache.calcite.sql.dialect.CalciteSqlDialect; import org.apache.calcite.sql.parser.SqlParseException; import org.apache.calcite.sql.pretty.SqlPrettyWriter; +import org.dbsp.sqlCompiler.compiler.frontend.SqlComment; +import org.dbsp.sqlCompiler.compiler.frontend.SqlCommentParser; +import org.dbsp.sqlCompiler.compiler.frontend.SqlPrettyPrinter; import org.apache.calcite.sql.util.SqlOperatorTables; import org.apache.calcite.sql.validate.SqlUserDefinedAggFunction; import org.dbsp.sqlCompiler.circuit.DBSPCircuit; @@ -98,7 +101,7 @@ import java.util.Comparator; import java.util.HashMap; import java.util.List; -import java.util.Locale; +import java.util.Objects; import java.util.Map; import java.util.regex.Pattern; @@ -532,7 +535,7 @@ boolean validateCreateIndex(CreateIndexStatement statement) { return true; } - void renameIdentifiers(List statements) { + void emitSql(List statements) { final PrintStream outputStream; try { @Nullable String outputFile = this.options.ioOptions.outputFile; @@ -547,20 +550,36 @@ void renameIdentifiers(List statements) { return; } + List comments = this.options.ioOptions.format + ? SqlCommentParser.parse(this.sources.getWholeProgram()) + : new ArrayList<>(); + IndentStream indentStream = new IndentStream(outputStream); + SqlPrettyPrinter printer = new SqlPrettyPrinter(indentStream, comments); + RenameIdentifiers renamer = new RenameIdentifiers(); - for (ParsedStatement stat: statements) { + for (ParsedStatement stat : statements) { if (stat.visible()) { - SqlNode renamed = renamer.visitNode(stat.statement()); - Utilities.enforce(renamed != null); - final SqlWriter sqlWriter = new SqlPrettyWriter( - SqlWriterConfig.of() - .withDialect(CalciteSqlDialect.DEFAULT) - .withQuoteAllIdentifiers(false) - ); - renamed.unparse(sqlWriter, 0, 0); - outputStream.println(sqlWriter + ";"); + SqlNode node = stat.statement(); + if (this.options.ioOptions.anonymize) + node = Objects.requireNonNull(renamer.visitNode(node)); + Utilities.enforce(node != null); + if (this.options.ioOptions.format) { + printer.print(node); + indentStream.append(";").newline(); + } else { + final SqlWriter sqlWriter = new SqlPrettyWriter( + SqlWriterConfig.of() + .withDialect(CalciteSqlDialect.DEFAULT) + .withQuoteAllIdentifiers(false) + ); + node.unparse(sqlWriter, 0, 0); + outputStream.println(sqlWriter + ";"); + } } } + if (this.options.ioOptions.format) { + printer.flushRemainingComments(); + } outputStream.close(); } @@ -569,8 +588,8 @@ void renameIdentifiers(List statements) { if (this.hasErrors()) return null; - if (this.options.ioOptions.anonymize) { - this.renameIdentifiers(parsed); + if (this.options.ioOptions.anonymize || this.options.ioOptions.format) { + this.emitSql(parsed); return null; } @@ -749,7 +768,8 @@ void renameIdentifiers(List statements) { return null; } - static final Pattern ITEM_ERROR = Pattern.compile("Cannot apply 'ITEM' to arguments of type 'ITEM\\(([^,]+), ([^']+)\\)'(.*)", Pattern.DOTALL); + static final Pattern ITEM_ERROR = Pattern.compile( + "Cannot apply 'ITEM' to arguments of type 'ITEM\\(([^,]+), ([^']+)\\)'(.*)", Pattern.DOTALL); static SourcePositionRange getRange(CalciteContextException e) { return new SourcePositionRange( @@ -769,23 +789,6 @@ private CompilationError improveErrorMessage(CalciteContextException e) { String newMessage = "Cannot apply indexing to arguments of type " + source + "[" + index + "]" + tail; return new CompilationError(newMessage, getRange(e)); } - if (message.contains("'TIMESTAMPDIFF'.")) { - // The Calcite parser replaces DATEDIFF with TIMESTAMPDIFF - // Try to figure out whether this is what happened and rewrite it. - // This heuristic may fail... but will work in general. - SourcePositionRange range = getRange(e); - String fragment = this.sources.getFragment(range, false); - String lower = fragment.toLowerCase(Locale.ENGLISH); - if (lower.contains("datediff") && !lower.contains("timestampdiff")) { - message = message.replace("TIMESTAMPDIFF", "DATEDIFF"); - return new CompilationError(message, range); - } - } - String newMessage = message.replace(":PEEK_NO_EXPAND", ""); - newMessage = newMessage.replace("RECORDTYPE", "ROW"); - if (!newMessage.equals(message)) { - return new CompilationError(newMessage, getRange(e)); - } } return new CompilationError(e); } diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/ProgramMetadata.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/ProgramMetadata.java index 7d4f55ae919..8d17e36f009 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/ProgramMetadata.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/ProgramMetadata.java @@ -45,7 +45,8 @@ private static String canonicalVariableName(String variable) { static final Set reserved = Set.of( DBSPCompiler.WARNINGS_ARE_ERRORS.toLowerCase(Locale.ENGLISH), - ProgramMetadata.AVOID_STAR_JOINS.toLowerCase(Locale.ENGLISH) + ProgramMetadata.AVOID_STAR_JOINS.toLowerCase(Locale.ENGLISH), + ProgramMetadata.ENFORCE_POSITIVE_INPUTS.toLowerCase(Locale.ENGLISH) ); private boolean known(String variable) { @@ -110,11 +111,20 @@ public boolean isExplicitlyOff(String variable) { } public static final String AVOID_STAR_JOINS = "FELDERA_AVOID_STAR_JOINS"; + /** When set to {@code true}, inserts a weight-validation check after every + * input table that has no primary key. */ + public static final String ENFORCE_POSITIVE_INPUTS = "ENFORCE_POSITIVE_INPUTS"; public boolean noStarJoins() { return this.isExplicitlyOn(AVOID_STAR_JOINS); } + /** Returns {@code true} if weight validation should be inserted after + * inputs without a primary key. */ + public boolean enforcePositiveInputs() { + return this.isExplicitlyOn(ENFORCE_POSITIVE_INPUTS); + } + /** True if a feature is not set or set to 'false' */ public boolean isOffOrMissing(String variable) { if (!this.hasValue(variable)) diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/backend/ToJsonOuterVisitor.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/backend/ToJsonOuterVisitor.java index aadcc399fef..c4a8ef31348 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/backend/ToJsonOuterVisitor.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/backend/ToJsonOuterVisitor.java @@ -24,6 +24,7 @@ import org.dbsp.sqlCompiler.circuit.operator.DBSPViewBaseOperator; import org.dbsp.sqlCompiler.circuit.operator.DBSPWindowOperator; import org.dbsp.sqlCompiler.circuit.operator.DBSPIntegrateTraceRetainValuesOperator; +import org.dbsp.sqlCompiler.circuit.operator.DBSPWeightValidatorOperator; import org.dbsp.sqlCompiler.compiler.DBSPCompiler; import org.dbsp.sqlCompiler.compiler.frontend.calciteCompiler.ProgramIdentifier; import org.dbsp.sqlCompiler.compiler.visitors.VisitDecision; @@ -241,6 +242,15 @@ public VisitDecision preorder(DBSPSourceMultisetOperator operator) { return this.preorder(operator.to(DBSPSourceTableOperator.class)); } + @Override + public VisitDecision preorder(DBSPWeightValidatorOperator operator) { + if (this.preorder(operator.to(DBSPSimpleOperator.class)).stop()) + return VisitDecision.STOP; + this.property("message"); + this.stream.append(operator.message); + return VisitDecision.CONTINUE; + } + @Override public VisitDecision preorder(DBSPSourceMapOperator operator) { if (this.preorder(operator.to(DBSPSourceTableOperator.class)).stop()) diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/backend/rust/ToRustVisitor.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/backend/rust/ToRustVisitor.java index 7423d93ff96..0539faa14a0 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/backend/rust/ToRustVisitor.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/backend/rust/ToRustVisitor.java @@ -968,6 +968,24 @@ VisitDecision retainOperator(DBSPBinaryOperator operator) { return VisitDecision.STOP; } + @Override + public VisitDecision preorder(DBSPWeightValidatorOperator operator) { + this.computeHash(operator); + DBSPType streamType = this.streamType(operator); + this.writeComments(operator) + .append("let ") + .append(operator.getNodeName(this.preferHash)) + .append(": "); + streamType.accept(this.innerVisitor); + this.builder.append(" = ") + .append(this.getInputName(operator, 0)) + .append(".integral_weight_validator(") + .append(Utilities.doubleQuote(operator.message, false)) + .append(");"); + this.tagStream(operator); + return VisitDecision.STOP; + } + @Override public VisitDecision preorder(DBSPInternOperator operator) { this.writeComments(operator) diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/ExpressionCompiler.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/ExpressionCompiler.java index ef59edbf0bc..39b08a50299 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/ExpressionCompiler.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/ExpressionCompiler.java @@ -107,6 +107,7 @@ import org.dbsp.sqlCompiler.ir.type.primitive.DBSPTypeBaseType; import org.dbsp.sqlCompiler.ir.type.primitive.DBSPTypeDate; import org.dbsp.sqlCompiler.ir.type.primitive.DBSPTypeFP; +import org.dbsp.sqlCompiler.ir.type.primitive.DBSPTypeKeyword; import org.dbsp.sqlCompiler.ir.type.primitive.DBSPTypeTime; import org.dbsp.sqlCompiler.ir.type.primitive.DBSPTypeTimestampTz; import org.dbsp.sqlCompiler.ir.type.primitive.DBSPTypeVariant; @@ -142,6 +143,7 @@ import java.nio.charset.Charset; import java.util.ArrayList; import java.util.List; +import java.util.Locale; import java.util.Objects; import java.util.UUID; @@ -818,6 +820,18 @@ void ensureInteger(CalciteObject node, List ops, int argument) { ops.set(argument, arg.cast(node, expected, DBSPCastExpression.CastType.SqlUnsafe)); } + /** Ensure that a specific operand is interpreted as a KEYWORD */ + void ensureKeyword(CalciteObject node, List ops, int argument) { + DBSPExpression arg = ops.get(argument); + if (arg.getType().sameType(DBSPTypeKeyword.INSTANCE)) + return; + if (arg.is(DBSPStringLiteral.class)) { + ops.set(argument, new DBSPKeywordLiteral(node, Objects.requireNonNull(arg.to(DBSPStringLiteral.class).value))); + return; + } + throw new CompilationError("Unknown keyword", node); + } + @SuppressWarnings("SameParameterValue") void nullLiteralToNullArray(List ops, int arg) { if (ops.get(arg).is(DBSPNullLiteral.class)) { @@ -1934,6 +1948,24 @@ else if (arg0Type.is(DBSPTypeMap.class)) this.ensureString(ops, 2); return compileKeywordFunction(call, node, null, type, ops, 0, 3); case TIMESTAMP_DIFF: + // Also called for DATEDIFF + this.ensureKeyword(node, ops, 0); + DBSPKeywordLiteral lit = ops.get(0).to(DBSPKeywordLiteral.class); + if (call.op.getName().toLowerCase(Locale.ENGLISH).equals("datediff")) { + switch (lit.keyword) { + // These units would overflow in 32 bits + case "minute": + case "second": + case "millisecond": + case "microsecond": + case "nanosecond": + throw new CompilationError("Use TIMESTAMPDIFF instead of DATEDIFF for " + + lit.keyword.toUpperCase(Locale.ENGLISH) + + "\n(TIMESTAMPDIFF result is BIGINT)", node); + default: + break; + } + } return compileKeywordFunction(call, node, null, type, ops, 0, 3); case TUMBLE: { validateArgCount(node, operationName, ops.size(), 2, 3); diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/SqlComment.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/SqlComment.java new file mode 100644 index 00000000000..40eefd0d16d --- /dev/null +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/SqlComment.java @@ -0,0 +1,32 @@ +package org.dbsp.sqlCompiler.compiler.frontend; + +import org.apache.calcite.sql.parser.SqlParserPos; + +/** A SQL comment extracted from the original source text, with its source position. */ +public class SqlComment { + public enum Kind { + /** {@code -- ...} to end of line (SQL standard). */ + LINE_DASH, + /** {@code // ...} to end of line (C++ style). */ + LINE_SLASH, + /** {@code /* ... *\/} possibly spanning multiple lines. */ + BLOCK + } + + public final Kind kind; + /** Full comment text, including delimiters (e.g. {@code -- foo\n} or {@code /* bar *\/}). */ + public final String text; + /** Position of the first character of the comment in the original source. */ + public final SqlParserPos pos; + + public SqlComment(Kind kind, String text, SqlParserPos pos) { + this.kind = kind; + this.text = text; + this.pos = pos; + } + + @Override + public String toString() { + return pos + ": " + text.trim(); + } +} diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/SqlCommentParser.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/SqlCommentParser.java new file mode 100644 index 00000000000..67a82237eae --- /dev/null +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/SqlCommentParser.java @@ -0,0 +1,194 @@ +package org.dbsp.sqlCompiler.compiler.frontend; + +import org.apache.calcite.sql.parser.SqlParserPos; + +import java.util.ArrayList; +import java.util.List; + +import static java.util.Objects.requireNonNull; + +/** + * Scans SQL source text for comments, recording each comment's text and + * position in the original source. + * + *

Correctly skips string literals ({@code '...'}), double-quoted + * identifiers ({@code "..."}), and backtick-quoted identifiers so that + * comment-like sequences inside them are not mistaken for real comments. + * + *

Recognised comment forms: + *

    + *
  • {@code --} to end of line (SQL standard single-line comment)
    • + *
  • {@code //} to end of line (C++-style; also accepted by Calcite)
    • + *
  • {@code /* ... *\/} (block comment, may span multiple lines)
    • + *
+ * + *

Calcite query hints ({@code /*+ ... *\/}) are not included in the + * output: they are already represented in the {@code SqlNode} tree and handled + * separately by {@link SqlPrettyPrinter}. + * + *

The input is assumed to be syntactically valid SQL (as accepted by + * Calcite). Unterminated block comments or string literals are consumed + * silently to end-of-input; the resulting output may be malformed. + */ +public class SqlCommentParser { + private final String sql; // source text being scanned + private final int n; // cached length of sql + private int i; // scan cursor (index into sql) + private int line; // current line number, 1-based + private int col; // current column number, 1-based + + private SqlCommentParser(final String sql) { + this.sql = sql; + this.n = sql.length(); + this.i = 0; + this.line = 1; + this.col = 1; + } + + /** Extract all comments from {@code sql}, in source order. */ + public static List parse(final String sql) { + return new SqlCommentParser(sql).scan(); + } + + private List scan() { + final List result = new ArrayList<>(); + + while (this.i < this.n) { + final char c = this.current(); + + // single-line comment: -- or // + if ((c == '-' && this.peekNext() == '-') + || (c == '/' && this.peekNext() == '/')) { + final SqlComment.Kind kind = (c == '-') ? SqlComment.Kind.LINE_DASH : SqlComment.Kind.LINE_SLASH; + final int start = this.i; + final SqlParserPos pos = new SqlParserPos(this.line, this.col); + this.nextChar(); // consume first marker char + this.nextChar(); // consume second marker char + while (this.i < this.n && this.current() != '\n' && this.current() != '\r') { + this.nextChar(); + } + if (this.i < this.n) { + this.nextChar(); // consume line terminator; \r\n handled as one unit + } + result.add(new SqlComment(kind, requireNonNull(this.sql.substring(start, this.i)), pos)); + continue; + } + + // block comment: /* ... */ + // Calcite query hints /*+ ... */ are not collected: they are already + // represented in the SqlNode tree and emitted by SqlPrettyPrinter separately. + if (c == '/' && this.peekNext() == '*') { + final int start = this.i; + final SqlParserPos pos = new SqlParserPos(this.line, this.col); + this.nextChar(); // consume '/' + this.nextChar(); // consume '*' + final boolean isHint = this.i < this.n && this.current() == '+'; + while (this.i < this.n) { + if (this.current() == '*' && this.peekNext() == '/') { + this.nextChar(); // consume '*' + this.nextChar(); // consume '/' + break; + } + this.nextChar(); + } + if (!isHint) { + result.add(new SqlComment(SqlComment.Kind.BLOCK, requireNonNull(this.sql.substring(start, this.i)), pos)); + } + continue; + } + + // single-quoted string literal: '...' + if (c == '\'') { + this.nextChar(); // consume opening ' + while (this.i < this.n) { + if (this.current() == '\'') { + this.nextChar(); // consume ' + if (this.i < this.n && this.current() == '\'') { + this.nextChar(); // '' is an escaped single quote; keep scanning + } else { + break; + } + } else { + this.nextChar(); + } + } + continue; + } + + // double-quoted identifier: "..." + if (c == '"') { + this.nextChar(); // consume opening " + while (this.i < this.n) { + if (this.current() == '"') { + this.nextChar(); // consume " + if (this.i < this.n && this.current() == '"') { + this.nextChar(); // "" is an escaped double-quote; keep scanning + } else { + break; + } + } else { + this.nextChar(); + } + } + continue; + } + + // backtick-quoted identifier: `...` + if (c == '`') { + this.nextChar(); // consume opening ` + while (this.i < this.n) { + if (this.current() == '`') { + this.nextChar(); // consume ` + if (this.i < this.n && this.current() == '`') { + this.nextChar(); // `` is an escaped backtick; keep scanning + } else { + break; + } + } else { + this.nextChar(); + } + } + continue; + } + + // ordinary character + this.nextChar(); + } + + return result; + } + + /** Returns the character at the current scan position; caller must ensure {@code i < n}. */ + private char current() { + return this.sql.charAt(this.i); + } + + /** Returns the character one position ahead of the cursor, or {@code 0} if out of bounds. */ + private char peekNext() { + final int next = this.i + 1; + return next < this.n ? this.sql.charAt(next) : 0; + } + + /** + * Consumes and returns the current character, advancing the cursor and + * updating line/column tracking. A {@code \r\n} pair counts as one line + * ending: this method skips the {@code \n} when the current character is + * {@code \r} and the next is {@code \n}. + */ + private char nextChar() { + final char c = this.sql.charAt(this.i++); + if (c == '\n') { + this.line++; + this.col = 1; + } else if (c == '\r') { + this.line++; + this.col = 1; + if (this.i < this.n && this.sql.charAt(this.i) == '\n') { + this.i++; // skip the \n of a \r\n pair + } + } else { + this.col++; + } + return c; + } +} diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/SqlPrettyPrinter.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/SqlPrettyPrinter.java new file mode 100644 index 00000000000..628c952ce01 --- /dev/null +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/SqlPrettyPrinter.java @@ -0,0 +1,579 @@ +package org.dbsp.sqlCompiler.compiler.frontend; + +import org.apache.calcite.sql.JoinConditionType; +import org.apache.calcite.sql.SqlBasicCall; +import org.apache.calcite.sql.SqlJoin; +import org.apache.calcite.sql.SqlKind; +import org.apache.calcite.sql.SqlLiteral; +import org.apache.calcite.sql.SqlNode; +import org.apache.calcite.sql.SqlNodeList; +import org.apache.calcite.sql.SqlOperator; +import org.apache.calcite.sql.SqlOrderBy; +import org.apache.calcite.sql.SqlSelect; +import org.apache.calcite.sql.SqlSelectKeyword; +import org.apache.calcite.sql.SqlWith; +import org.apache.calcite.sql.SqlWithItem; +import org.apache.calcite.sql.SqlWriterConfig; +import org.apache.calcite.sql.dialect.CalciteSqlDialect; +import org.apache.calcite.sql.fun.SqlCase; +import org.apache.calcite.sql.parser.SqlParserPos; +import org.checkerframework.checker.nullness.qual.Nullable; +import org.dbsp.sqlCompiler.compiler.frontend.parser.SqlCreateTable; +import org.dbsp.sqlCompiler.compiler.frontend.parser.SqlCreateView; +import org.dbsp.util.IIndentStream; +import org.dbsp.util.IndentStream; +import org.dbsp.util.Utilities; + +import java.util.ArrayList; +import java.util.List; +import java.util.Objects; +import java.util.function.UnaryOperator; + +import static java.util.Objects.requireNonNull; + +/** Formats a {@link SqlNode} tree as SQL text using an {@link IIndentStream}. */ +// Calcite has an unparse method, but it has lots of options and bugs. This printer has no options. +public class SqlPrettyPrinter { + private static final UnaryOperator UNQUOTED_CONFIG = + c -> c.withDialect(CalciteSqlDialect.DEFAULT).withQuoteAllIdentifiers(false); + + /** + * Wraps a sorted list of {@link SqlComment}s with a cursor. + * Given a position, {@link #before(SqlParserPos)} returns every comment + * whose source position strictly precedes that position, then advances + * the cursor past those comments. + */ + private static class CommentStream { + private final List comments; + private int cursor; + + CommentStream(List comments) { + this.comments = comments; + this.cursor = 0; + } + + /** Returns {@code true} if {@code a} appears strictly before {@code b} in the source. */ + private static boolean isBefore(SqlParserPos a, SqlParserPos b) { + int aLine = a.getLineNum(); + int bLine = b.getLineNum(); + if (aLine != bLine) return aLine < bLine; + return a.getColumnNum() < b.getColumnNum(); + } + + /** + * Returns all comments whose source position is strictly before {@code pos}, + * advancing the cursor past them. + */ + List before(SqlParserPos pos) { + List result = new ArrayList<>(); + while (this.cursor < this.comments.size()) { + SqlComment c = requireNonNull(this.comments.get(this.cursor)); + if (!isBefore(requireNonNull(c.pos), pos)) break; + result.add(c); + this.cursor++; + } + return result; + } + + /** Returns all remaining comments and advances the cursor to the end. */ + List remaining() { + List result = new ArrayList<>(this.comments.subList(this.cursor, this.comments.size())); + this.cursor = this.comments.size(); + return result; + } + } + + private final IIndentStream stream; + private final CommentStream commentStream; + + public SqlPrettyPrinter(IIndentStream stream, List comments) { + this.stream = stream; + this.commentStream = new CommentStream(comments); + } + + public SqlPrettyPrinter(IIndentStream stream) { + this(stream, new ArrayList<>()); + } + + /** + * Emit all comments whose source line precedes {@code node}'s start position, + * then advance the comment cursor past them. + */ + private void flushBefore(SqlNode node) { + for (SqlComment comment : this.commentStream.before(requireNonNull(node.getParserPosition()))) { + this.stream.append(requireNonNull(comment.text.stripTrailing())).newline(); + } + } + + /** Emit all remaining comments that follow every printed node. */ + public void flushRemainingComments() { + for (SqlComment comment : this.commentStream.remaining()) { + this.stream.append(requireNonNull(comment.text.stripTrailing())).newline(); + } + } + + /** Format {@code node} as a SQL string. */ + public static String toString(SqlNode node) { + StringBuilder sb = new StringBuilder(); + IndentStream stream = new IndentStream(sb); + new SqlPrettyPrinter(stream).print(node); + return sb.toString(); + } + + public static String toString(SqlNodeList nodes) { + return toString(nodes, List.of()); + } + + /** + * Format a list of SQL statements, interleaving comments from the original source + * at the correct positions — both between statements and inside them (e.g., between + * columns of a CREATE TABLE). + */ + public static String toString(SqlNodeList nodes, List comments) { + StringBuilder sb = new StringBuilder(); + IndentStream stream = new IndentStream(sb); + SqlPrettyPrinter printer = new SqlPrettyPrinter(stream, comments); + for (SqlNode node : nodes) { + printer.print(requireNonNull(node)); + stream.append(";").newline(); + } + printer.flushRemainingComments(); + return sb.toString(); + } + + /** Get the SQL using the Calcite implementation */ + private String fallbackSql(SqlNode node) { + return node.toSqlString(UNQUOTED_CONFIG).getSql(); + } + + /** Append the SQL text for {@code node} to the this.stream. */ + public void print(@Nullable SqlNode node) { + if (node == null) return; + this.flushBefore(node); + + // Feldera-specific DDL nodes (check before SqlCall) + if (node instanceof SqlCreateView) { this.printCreateView((SqlCreateView) node); return; } + if (node instanceof SqlCreateTable) { this.printCreateTable((SqlCreateTable) node); return; } + // Standard Calcite query nodes + if (node instanceof SqlSelect) { this.printSelect((SqlSelect) node); return; } + if (node instanceof SqlOrderBy) { this.printOrderBy((SqlOrderBy) node); return; } + if (node instanceof SqlWith) { this.printWith((SqlWith) node); return; } + if (node instanceof SqlJoin) { this.printJoin((SqlJoin) node); return; } + + if (node instanceof SqlCase) { this.printCase((SqlCase) node); return; } + if (node instanceof SqlBasicCall) { this.printBasicCall((SqlBasicCall) node); return; } + + // Fallback: use Calcite's own unparse without forcing quotes on all identifiers + this.stream.append(this.fallbackSql(node)); + } + + /** + * Print a node that appears inside an expression context. + * A SELECT sub-query is wrapped in parentheses and indented. + */ + private void printExpr(@Nullable SqlNode node) { + if (node == null) return; + SqlKind kind = node.getKind(); + if (node instanceof SqlSelect + || node instanceof SqlOrderBy + || node instanceof SqlWith + || kind == SqlKind.UNION + || kind == SqlKind.INTERSECT + || kind == SqlKind.EXCEPT + || kind == SqlKind.VALUES) { + this.printQueryParens(node); + } else { + this.print(node); + } + } + + private void printQueryParens(SqlNode query) { + this.stream.append("(").increase(); + this.print(query); + this.stream.decrease().newline().append(")"); + } + + /** Print the RHS of an IN / NOT IN expression. + * A value list {@code (1, 2, 3)} is a {@link SqlNodeList} and needs explicit parens. + * A subquery is handled by {@link #printExpr}. */ + private void printInList(SqlNode rhs) { + if (rhs instanceof SqlNodeList list) { + this.stream.append("("); + for (int i = 0; i < list.size(); i++) { + if (i > 0) this.stream.append(", "); + this.printExpr(requireNonNull(list.get(i))); + } + this.stream.append(")"); + } else { + this.printExpr(rhs); + } + } + + private void printSelect(SqlSelect select) { + this.stream.append("SELECT"); + + if (select.hasHints()) { + this.stream.append(" /*+"); + SqlNodeList hints = requireNonNull(select.getHints()); + for (int i = 0; i < hints.size(); i++) { + if (i > 0) this.stream.append(","); + this.stream.append(" ").append(this.fallbackSql(requireNonNull(hints.get(i)))); + } + this.stream.append(" */"); + } + + if (select.isDistinctOn()) { + this.stream.append(" DISTINCT ON ("); + SqlNodeList distinctOn = requireNonNull(select.getDistinctOn()); + for (int i = 0; i < distinctOn.size(); i++) { + if (i > 0) this.stream.append(", "); + this.printExpr(requireNonNull(distinctOn.get(i))); + } + this.stream.append(")"); + } else if (select.isDistinct()) { + this.stream.append(" DISTINCT"); + } else if (select.getModifierNode(SqlSelectKeyword.ALL) != null) { + this.stream.append(" ALL"); + } + + // SELECT list — one item per line, indented + SqlNodeList selectList = select.getSelectList(); + if (selectList != null && !selectList.isEmpty()) { + this.stream.increase(); + for (int i = 0; i < selectList.size(); i++) { + if (i > 0) this.stream.append(",").newline(); + this.printExpr(requireNonNull(selectList.get(i))); + } + this.stream.decrease(); + } + + SqlNode from = select.getFrom(); + if (from != null) { + this.stream.newline().append("FROM "); + this.printFrom(from); + } + + SqlNode where = select.getWhere(); + if (where != null) { + this.stream.newline().append("WHERE "); + this.printExpr(where); + } + + SqlNodeList groupBy = select.getGroup(); + if (groupBy != null && !groupBy.isEmpty()) { + this.stream.newline().append("GROUP BY "); + for (int i = 0; i < groupBy.size(); i++) { + if (i > 0) this.stream.append(", "); + this.printExpr(groupBy.get(i)); + } + } + + SqlNode having = select.getHaving(); + if (having != null) { + this.stream.newline().append("HAVING "); + this.printExpr(having); + } + + SqlNodeList windowDecls = select.getWindowList(); + if (windowDecls != null && !windowDecls.isEmpty()) { + this.stream.newline().append("WINDOW "); + for (int i = 0; i < windowDecls.size(); i++) { + if (i > 0) this.stream.append(", "); + this.print(windowDecls.get(i)); + } + } + + SqlNode qualify = select.getQualify(); + if (qualify != null) { + this.stream.newline().append("QUALIFY "); + this.printExpr(qualify); + } + + SqlNodeList orderBy = select.getOrderList(); + if (orderBy != null && !orderBy.isEmpty()) { + this.stream.newline().append("ORDER BY "); + for (int i = 0; i < orderBy.size(); i++) { + if (i > 0) this.stream.append(", "); + this.printExpr(orderBy.get(i)); + } + } + + SqlNode offset = select.getOffset(); + if (offset != null) { + this.stream.newline().append("OFFSET "); + this.printExpr(offset); + } + + SqlNode fetch = select.getFetch(); + if (fetch != null) { + this.stream.newline().append("LIMIT "); + this.printExpr(fetch); + } + } + + private void printFrom(SqlNode from) { + if (from instanceof SqlJoin) { + this.printJoin((SqlJoin) from); + } else { + this.printExpr(from); + } + } + + private void printJoin(SqlJoin join) { + this.printFrom(join.getLeft()); + this.stream.newline(); + boolean natural = join.isNatural(); + switch (join.getJoinType()) { + case INNER: this.stream.append(natural ? "NATURAL INNER JOIN " : "INNER JOIN "); break; + case LEFT: this.stream.append(natural ? "NATURAL LEFT JOIN " : "LEFT JOIN "); break; + case RIGHT: this.stream.append(natural ? "NATURAL RIGHT JOIN " : "RIGHT JOIN "); break; + case FULL: this.stream.append(natural ? "NATURAL FULL JOIN " : "FULL JOIN "); break; + case CROSS: this.stream.append(natural ? "NATURAL CROSS JOIN " : "CROSS JOIN "); break; + case COMMA: this.stream.append(", "); break; + default: this.stream.append("JOIN "); break; + } + SqlNode rightSide = join.getRight(); + if (rightSide instanceof SqlJoin) { + // A nested join on the right must be parenthesized; otherwise + // "JOIN (B JOIN C ON c1) ON c2" would print as "JOIN B JOIN C ON c1 ON c2" + // which the parser rejects (second ON appears where a join type is expected). + this.stream.append("(").increase(); + this.printJoin((SqlJoin) rightSide); + this.stream.decrease().newline().append(")"); + } else { + this.printExpr(rightSide); + } + JoinConditionType condType = join.getConditionType(); + SqlNode cond = join.getCondition(); + if (condType == JoinConditionType.ON && cond != null) { + this.stream.append(" ON "); + this.printExpr(cond); + } else if (condType == JoinConditionType.USING && cond != null) { + this.stream.append(" USING ("); + this.printExpr(cond); + this.stream.append(")"); + } + } + + private void printOrderBy(SqlOrderBy orderBy) { + this.print(requireNonNull(orderBy.query)); + if (!orderBy.orderList.isEmpty()) { + this.stream.newline().append("ORDER BY "); + for (int i = 0; i < orderBy.orderList.size(); i++) { + if (i > 0) this.stream.append(", "); + this.printExpr(orderBy.orderList.get(i)); + } + } + if (orderBy.offset != null) { + this.stream.newline().append("OFFSET "); + this.printExpr(orderBy.offset); + } + if (orderBy.fetch != null) { + this.stream.newline().append("LIMIT "); + this.printExpr(orderBy.fetch); + } + } + + private void printWith(SqlWith with) { + this.stream.append("WITH").increase(); + for (int i = 0; i < with.withList.size(); i++) { + if (i > 0) this.stream.append(",").newline(); + SqlWithItem item = (SqlWithItem) with.withList.get(i); + this.stream.append(this.fallbackSql(requireNonNull(item.name))); + this.stream.append(" AS "); + this.printQueryParens(item.query); + } + this.stream.decrease().newline(); + this.print(with.body); + } + + private void printCase(SqlCase caseExpr) { + SqlNode value = caseExpr.getValueOperand(); + if (value != null) { + this.stream.append("CASE "); + this.printExpr(value); + } else { + this.stream.append("CASE"); + } + SqlNodeList whenList = caseExpr.getWhenOperands(); + SqlNodeList thenList = caseExpr.getThenOperands(); + for (int i = 0; i < whenList.size(); i++) { + this.stream.append(" WHEN "); + this.printExpr(requireNonNull(whenList.get(i))); + this.stream.append(" THEN "); + this.printExpr(requireNonNull(thenList.get(i))); + } + SqlNode elseExpr = caseExpr.getElseOperand(); + if (elseExpr != null) { + this.stream.append(" ELSE "); + this.printExpr(elseExpr); + } + this.stream.append(" END"); + } + + private void printBasicCall(SqlBasicCall call) { + SqlOperator op = call.getOperator(); + List<@Nullable SqlNode> operands = call.getOperandList(); + SqlKind kind = op.getKind(); + + switch (kind) { + case AS: + this.printExpr(requireNonNull(operands.get(0))); + this.stream.append(" AS "); + // Alias is always a simple identifier — no parens needed + this.print(operands.get(1)); + return; + case CAST: { + this.stream.append("CAST("); + this.printExpr(requireNonNull(operands.get(0))); + this.stream.append(" AS "); + SqlNode castType = requireNonNull(operands.get(1)); + if (castType instanceof org.apache.calcite.sql.SqlIntervalQualifier) { + this.stream.append("INTERVAL "); + } + this.print(castType); + this.stream.append(")"); + return; + } + case DESCENDING: + this.printExpr(requireNonNull(operands.get(0))); + this.stream.append(" DESC"); + return; + case NULLS_FIRST: + this.printExpr(requireNonNull(operands.get(0))); + this.stream.append(" NULLS FIRST"); + return; + case NULLS_LAST: + this.printExpr(requireNonNull(operands.get(0))); + this.stream.append(" NULLS LAST"); + return; + case IN: + this.printExpr(requireNonNull(operands.get(0))); + this.stream.append(" IN "); + this.printInList(requireNonNull(operands.get(1))); + return; + case NOT_IN: + this.printExpr(requireNonNull(operands.get(0))); + this.stream.append(" NOT IN "); + this.printInList(requireNonNull(operands.get(1))); + return; + case LATERAL: + this.stream.append("LATERAL "); + this.printExpr(requireNonNull(operands.get(0))); + return; + case FILTER: + this.printExpr(requireNonNull(operands.get(0))); + this.stream.append(" FILTER (WHERE "); + this.printExpr(requireNonNull(operands.get(1))); + this.stream.append(")"); + return; + case WITHIN_GROUP: { + this.printExpr(requireNonNull(operands.get(0))); + this.stream.append(" WITHIN GROUP (ORDER BY "); + SqlNodeList orderList = (SqlNodeList) requireNonNull(operands.get(1)); + for (int i = 0; i < orderList.size(); i++) { + if (i > 0) this.stream.append(", "); + this.printExpr(requireNonNull(orderList.get(i))); + } + this.stream.append(")"); + return; + } + default: + break; + } + + if (operands.size() == 2 && op.getSyntax() == org.apache.calcite.sql.SqlSyntax.BINARY) { + this.printExpr(operands.get(0)); + this.stream.append(" ").append(op.getName()).append(" "); + this.printExpr(operands.get(1)); + return; + } else if (operands.size() == 1 && op.getSyntax() == org.apache.calcite.sql.SqlSyntax.POSTFIX) { + this.printExpr(operands.get(0)); + this.stream.append(" ").append(op.getName()); + return; + } else if (operands.size() == 1 && op.getSyntax() == org.apache.calcite.sql.SqlSyntax.PREFIX) { + this.stream.append(op.getName()).append(" "); + this.printExpr(operands.get(0)); + return; + } + + // Fallback: let Calcite's own unparse() handle everything else + this.stream.append(this.fallbackSql(call)); + } + + private void printCreateView(SqlCreateView view) { + if (view.getReplace()) { + this.stream.append("CREATE OR REPLACE"); + } else { + this.stream.append("CREATE"); + } + switch (view.viewKind) { + case LOCAL: this.stream.append(" LOCAL"); break; + case MATERIALIZED: this.stream.append(" MATERIALIZED"); break; + default: break; + } + this.stream.append(" VIEW "); + this.stream.append(this.fallbackSql(requireNonNull(view.name))); + + SqlNodeList cols = view.columnList; + if (cols != null && !cols.isEmpty()) { + this.stream.append("("); + for (int i = 0; i < cols.size(); i++) { + if (i > 0) this.stream.append(", "); + this.print(cols.get(i)); + } + this.stream.append(")"); + } + + this.printProperties(view.viewProperties); + this.stream.append(" AS").newline(); + this.print(view.query); + } + + private void printCreateTable(SqlCreateTable table) { + this.stream.append("CREATE TABLE "); + if (table.ifNotExists) this.stream.append("IF NOT EXISTS "); + this.stream.append(this.fallbackSql(table.name)); + this.stream.append(" (").increase(); + for (int i = 0; i < table.columnsOrForeignKeys.size(); i++) { + if (i > 0) this.stream.append(",").newline(); + this.print(requireNonNull(table.columnsOrForeignKeys.get(i))); + } + this.stream.decrease().newline().append(")"); + this.printProperties(table.tableProperties); + } + + /** Format a WITH (key = value, ...) property list. */ + private void printProperties(@Nullable SqlNodeList properties) { + if (properties == null || properties.isEmpty()) return; + this.stream.append(" WITH (").increase(); + for (int i = 0; i < properties.size(); i += 2) { + if (i > 0) this.stream.append(",").newline(); + SqlNode keyNode = requireNonNull(properties.get(i)); + SqlNode valueNode = requireNonNull(properties.get(i + 1)); + this.print(keyNode); + this.stream.append(" = "); + + // Pretty-print the 'connectors' value as formatted JSON + boolean emitted = false; + if (keyNode instanceof SqlLiteral key + && valueNode instanceof SqlLiteral value + && Objects.equals(key.toValue(), "connectors")) { + String raw = value.toValue(); + if (raw == null) { + this.stream.append("null"); + } else { + String formatted = Utilities.tryFormatAsJson(raw, this.stream.getIndentAmount()); + if (!formatted.equals(raw)) { + this.stream.append("'" + formatted.replace("'", "''") + "'"); + emitted = true; + } + } + } + if (!emitted) { + this.print(valueNode); + } + } + this.stream.decrease().newline().append(")"); + } +} diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/calciteCompiler/SqlToRelCompiler.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/calciteCompiler/SqlToRelCompiler.java index 77cbf11c38a..57782723dfb 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/calciteCompiler/SqlToRelCompiler.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/calciteCompiler/SqlToRelCompiler.java @@ -141,6 +141,9 @@ import org.dbsp.sqlCompiler.compiler.errors.UnimplementedException; import org.dbsp.sqlCompiler.compiler.errors.UnsupportedException; import org.dbsp.sqlCompiler.compiler.frontend.ExtendedSqlParserPos; +import org.dbsp.sqlCompiler.compiler.frontend.SqlComment; +import org.dbsp.sqlCompiler.compiler.frontend.SqlCommentParser; +import org.dbsp.sqlCompiler.compiler.frontend.SqlPrettyPrinter; import org.dbsp.sqlCompiler.compiler.frontend.calciteCompiler.optimizer.CalciteOptimizer; import org.dbsp.sqlCompiler.compiler.frontend.calciteObject.CalciteObject; import org.dbsp.sqlCompiler.compiler.frontend.parser.PropertyList; @@ -847,9 +850,18 @@ public SqlNode parse(String sql) throws SqlParseException { /** Given a list of statements separated by semicolons, parse all of them. * @param saveLines True if the lines are from the user program; false if they are internally generated. */ public List parseStatements(String statements, boolean saveLines) throws SqlParseException { + // Debugging flag for (partially) testing SqlPrettyPrinter + final boolean ROUND_TRIP_CHECK = false; + SqlParser sqlParser = this.createSqlParser(statements, saveLines); List result = new ArrayList<>(); SqlNodeList sqlNodes = sqlParser.parseStmtList(); + if (saveLines && ROUND_TRIP_CHECK) { + List comments = SqlCommentParser.parse(statements); + String sql = SqlPrettyPrinter.toString(sqlNodes, comments); + // System.out.println(sql); + this.createSqlParser(sql, false).parseStmtList(); + } for (SqlNode node: sqlNodes) { SqlNode sqlNode = this.postParsingProcess(node, saveLines); ParsedStatement stat = new ParsedStatement(sqlNode, saveLines); diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/parser/SqlCreateAggregate.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/parser/SqlCreateAggregate.java index 6fd559e5f99..d478a0e9fef 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/parser/SqlCreateAggregate.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/parser/SqlCreateAggregate.java @@ -76,6 +76,9 @@ public boolean isLinear() { writer.endList(frame); writer.keyword("RETURNS"); this.returnType.unparse(writer, 0, 0); + if (Boolean.FALSE.equals(this.returnType.getNullable())) { + writer.keyword("NOT NULL"); + } } @Override public SqlOperator getOperator() { diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/parser/SqlCreateFunctionDeclaration.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/parser/SqlCreateFunctionDeclaration.java index df28c40dc07..82f4f480ded 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/parser/SqlCreateFunctionDeclaration.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/parser/SqlCreateFunctionDeclaration.java @@ -71,6 +71,9 @@ public SqlCreateFunctionDeclaration(SqlParserPos pos, boolean replace, writer.endList(frame); writer.keyword("RETURNS"); this.returnType.unparse(writer, 0, 0); + if (Boolean.FALSE.equals(this.returnType.getNullable())) { + writer.keyword("NOT NULL"); + } if (this.body != null) { writer.keyword("AS"); this.body.unparse(writer, 0, 0); diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/parser/SqlLateness.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/parser/SqlLateness.java index 8dfd137bb98..d269b2f9ff0 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/parser/SqlLateness.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/parser/SqlLateness.java @@ -101,6 +101,7 @@ public SqlNode getLateness() { final int opLeft = getOperator().getLeftPrec(); final int opRight = getOperator().getRightPrec(); this.view.unparse(writer, opLeft, opRight); + writer.print("."); this.column.unparse(writer, opLeft, opRight); this.expression.unparse(writer, 0, 0); writer.newlineAndIndent(); diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/visitors/outer/CircuitCloneVisitor.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/visitors/outer/CircuitCloneVisitor.java index effc7e6e73b..49a4e206be9 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/visitors/outer/CircuitCloneVisitor.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/visitors/outer/CircuitCloneVisitor.java @@ -313,6 +313,11 @@ public void postorder(DBSPIntegrateOperator operator) { this.replace(operator); } + @Override + public void postorder(DBSPWeightValidatorOperator operator) { + this.replace(operator); + } + @Override public void postorder(DBSPUpsertFeedbackOperator operator) { this.replace(operator); diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/visitors/outer/CircuitOptimizer.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/visitors/outer/CircuitOptimizer.java index 525ead4d9f2..3f8e50a3878 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/visitors/outer/CircuitOptimizer.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/visitors/outer/CircuitOptimizer.java @@ -56,7 +56,7 @@ public CircuitOptimizer(DBSPCompiler compiler) { super("Optimizer", compiler); this.createOptimizer(); } - + static class StopOnError extends CircuitVisitor { public StopOnError(DBSPCompiler compiler) { super(compiler); @@ -126,6 +126,8 @@ void createOptimizer() { this.add(new OptimizeWithGraph(compiler, g -> new CloneOperatorsWithFanout(compiler, g))); this.add(new LinearPostprocessRetainKeys(compiler)); this.add(new ExpandIndexedInputs(compiler)); + this.add(new Conditional(compiler, new InsertWeightValidation(compiler), + this.compiler.metadata::enforcePositiveInputs)); this.add(new OptimizeWithGraph(compiler, g -> new FilterJoinVisitor(compiler, g))); this.add(new DeadCode(compiler, true)); this.add(new Simplify(compiler).circuitRewriter(true)); diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/visitors/outer/CircuitVisitor.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/visitors/outer/CircuitVisitor.java index ee3a8d9aca1..7d7067208c3 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/visitors/outer/CircuitVisitor.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/visitors/outer/CircuitVisitor.java @@ -302,6 +302,10 @@ public VisitDecision preorder(DBSPIntegrateOperator node) { return this.preorder((DBSPUnaryOperator) node); } + public VisitDecision preorder(DBSPWeightValidatorOperator node) { + return this.preorder((DBSPUnaryOperator) node); + } + public VisitDecision preorder(DBSPUpsertFeedbackOperator node) { return this.preorder((DBSPUnaryOperator) node); } @@ -677,6 +681,10 @@ public void postorder(DBSPIntegrateOperator node) { this.postorder((DBSPUnaryOperator) node); } + public void postorder(DBSPWeightValidatorOperator node) { + this.postorder((DBSPUnaryOperator) node); + } + public void postorder(DBSPUpsertFeedbackOperator node) { this.postorder((DBSPUnaryOperator) node); } diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/visitors/outer/InsertWeightValidation.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/visitors/outer/InsertWeightValidation.java new file mode 100644 index 00000000000..cf68711c272 --- /dev/null +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/visitors/outer/InsertWeightValidation.java @@ -0,0 +1,22 @@ +package org.dbsp.sqlCompiler.compiler.visitors.outer; + +import org.dbsp.sqlCompiler.circuit.operator.DBSPSourceMultisetOperator; +import org.dbsp.sqlCompiler.circuit.operator.DBSPWeightValidatorOperator; +import org.dbsp.sqlCompiler.compiler.DBSPCompiler; + +/** Inserts a {@link DBSPWeightValidatorOperator} after every + * {@link DBSPSourceMultisetOperator} (input tables without a primary key). */ +public class InsertWeightValidation extends CircuitCloneVisitor { + public InsertWeightValidation(DBSPCompiler compiler) { + super(compiler, false); + } + + @Override + public void postorder(DBSPSourceMultisetOperator node) { + super.replace(node); + DBSPWeightValidatorOperator validator = new DBSPWeightValidatorOperator( + node.getRelNode(), this.mapped(node.outputPort()), "Table " + node.tableName.name()); + this.remap.put(node.outputPort(), validator.outputPort()); + this.addOperator(validator); + } +} diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/ir/expression/literal/DBSPKeywordLiteral.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/ir/expression/literal/DBSPKeywordLiteral.java index 9dd8fea9a78..90dc7152eb3 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/ir/expression/literal/DBSPKeywordLiteral.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/ir/expression/literal/DBSPKeywordLiteral.java @@ -48,8 +48,31 @@ public boolean sameValue(@Nullable ISameValue o) { public DBSPKeywordLiteral(CalciteObject node, String keyword) { super(node, DBSPTypeKeyword.INSTANCE, false); - this.keyword = keyword.toLowerCase(); switch (keyword.toLowerCase()) { + case "sql_tsi_year": + keyword = "year"; + break; + case "sql_tsi_quarter": + keyword = "quarter"; + break; + case "sql_tsi_month": + keyword = "month"; + break; + case "sql_tsi_week": + keyword = "week"; + break; + case "sql_tsi_day": + keyword = "day"; + break; + case "sql_tsi_hour": + keyword = "hour"; + break; + case "sql_tsi_second": + keyword = "second"; + break; + case "sql_tsi_minute": + keyword = "minute"; + break; case "dow": case "epoch": case "isodow": @@ -77,6 +100,7 @@ public DBSPKeywordLiteral(CalciteObject node, String keyword) { default: throw new UnimplementedException("Not yet implemented support for Calcite construct " + keyword, node); } + this.keyword = keyword.toLowerCase(); } @Override diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/util/IIndentStream.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/util/IIndentStream.java index 23c1402fa5a..11bedfa460c 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/util/IIndentStream.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/util/IIndentStream.java @@ -224,6 +224,9 @@ default IIndentStream newline() { IIndentStream increase(); IIndentStream decrease(); + /** Return the number of spaces added per indentation level. */ + int getIndentAmount(); + default IIndentStream appendJsonLabelAndColon(String label) { return this.append("\"").append(Utilities.escapeDoubleQuotes(label)).append("\": "); } diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/util/IndentStream.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/util/IndentStream.java index 486af0b037b..339d12fc124 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/util/IndentStream.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/util/IndentStream.java @@ -51,6 +51,9 @@ public Appendable setOutputStream(Appendable appendable) { return result; } + @Override + public int getIndentAmount() { return this.amount; } + /** Set the indent amount. If less or equal to 0, newline will have no effect. */ public IIndentStream setIndentAmount(int amount) { Utilities.enforce(amount >= 0); diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/util/NullIndentStream.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/util/NullIndentStream.java index a2e461fd8b1..2088215e5b8 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/util/NullIndentStream.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/util/NullIndentStream.java @@ -151,6 +151,9 @@ public IIndentStream newline() { return this; } + @Override + public int getIndentAmount() { return 0; } + @Override public IIndentStream increase() { return this; diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/util/Utilities.java b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/util/Utilities.java index 57f9119cb88..fbf2c209021 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/util/Utilities.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/util/Utilities.java @@ -29,6 +29,8 @@ import com.fasterxml.jackson.core.JsonLocation; import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.core.util.DefaultIndenter; +import com.fasterxml.jackson.core.util.DefaultPrettyPrinter; import com.fasterxml.jackson.databind.DeserializationFeature; import com.fasterxml.jackson.databind.JsonMappingException; import com.fasterxml.jackson.databind.JsonNode; @@ -226,6 +228,24 @@ public static ObjectMapper deterministicObjectMapper() { .build(); } + /** + * If {@code value} is valid JSON, returns it pretty-printed using {@code indent} + * spaces per indentation level. + * Returns the original string unchanged when the value is not valid JSON. + */ + public static String tryFormatAsJson(String value, int indent) { + try { + Object parsed = deterministicObjectMapper().readValue(value, Object.class); + DefaultPrettyPrinter printer = new DefaultPrettyPrinter(); + // Arrays use NopIndenter so "[" and "{" appear on the same line as the key. + printer.indentArraysWith(DefaultPrettyPrinter.NopIndenter.instance); + printer.indentObjectsWith(new DefaultIndenter(" ".repeat(indent), "\n")); + return deterministicObjectMapper().writer(printer).writeValueAsString(parsed); + } catch (Exception e) { + return value; + } + } + /** Add double quotes around string and escape symbols that need it. * @param unicodeBraces If true use \\u{} for Unicode, otherwise use \\u. */ public static String doubleQuote(String value, boolean unicodeBraces) { diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/MetadataTests.java b/sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/MetadataTests.java index 81ce87bbc41..bc1434c6e23 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/MetadataTests.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/MetadataTests.java @@ -12,6 +12,8 @@ import org.dbsp.sqlCompiler.compiler.TestUtil; import org.dbsp.sqlCompiler.compiler.backend.JsonDecoder; import org.dbsp.sqlCompiler.compiler.errors.CompilerMessages; +import org.dbsp.sqlCompiler.compiler.frontend.SqlComment; +import org.dbsp.sqlCompiler.compiler.frontend.SqlCommentParser; import org.dbsp.sqlCompiler.compiler.frontend.calciteCompiler.ProgramIdentifier; import org.dbsp.sqlCompiler.compiler.frontend.statements.CreateTableStatement; import org.dbsp.sqlCompiler.compiler.frontend.statements.DeclareViewStatement; @@ -1201,6 +1203,9 @@ public void testHelpMessage() throws SQLException { --errors Error output file; stderr if not specified Default: + --format + Output the SQL program reformatted + Default: false --handles Use handles (true) or Catalog (false) in the emitted Rust code Default: false @@ -2064,6 +2069,270 @@ SELECT jsonstring_as_ID_0(ID_7), ID_6, SUM(ID_4.ID_5) AS ID_9, COUNT(ID_4.ID_1[1 Utilities.deleteFile(input, false); } + // Helpers for SqlCommentParser unit tests + static void assertComment(SqlComment c, SqlComment.Kind kind, String text, int line, int col) { + Assert.assertEquals(kind, c.kind); + Assert.assertEquals(text, c.text); + Assert.assertEquals(line, c.pos.getLineNum()); + Assert.assertEquals(col, c.pos.getColumnNum()); + } + + /** Verify that {@code sql} is accepted by the SQL compiler without errors. */ + private void assertCompilesWithoutError(String sql) { + DBSPCompiler compiler = this.testCompiler(); + compiler.submitStatementsForCompilation(sql); + getCircuit(compiler); + Assert.assertEquals(0, compiler.messages.exitCode); + } + + @Test + public void testCommentParserLineDash() { + List cs = SqlCommentParser.parse("-- hello\n"); + Assert.assertEquals(1, cs.size()); + assertComment(cs.get(0), SqlComment.Kind.LINE_DASH, "-- hello\n", 1, 1); + } + + @Test + public void testCommentParserLineSlash() { + // "CREATE VIEW V AS SELECT 1; " is 27 chars, so "//" is at col 28. + String sql = "CREATE VIEW V AS SELECT 1; // end\n"; + List cs = SqlCommentParser.parse(sql); + Assert.assertEquals(1, cs.size()); + assertComment(cs.get(0), SqlComment.Kind.LINE_SLASH, "// end\n", 1, 28); + assertCompilesWithoutError(sql); + } + + @Test + public void testCommentParserBlock() { + List cs = SqlCommentParser.parse("/* hello */"); + Assert.assertEquals(1, cs.size()); + assertComment(cs.get(0), SqlComment.Kind.BLOCK, "/* hello */", 1, 1); + } + + @Test + public void testCommentParserNoTrailingNewline() { + // Single-line comment at EOF with no newline: text has no trailing newline. + String sql = "CREATE VIEW V AS SELECT 1; -- end"; + List cs = SqlCommentParser.parse(sql); + Assert.assertEquals(1, cs.size()); + Assert.assertEquals("-- end", cs.get(0).text); + assertCompilesWithoutError(sql); + } + + @Test + public void testCommentParserMultipleComments() { + // Line 2: "CREATE VIEW V AS SELECT 1; " is 27 chars, so "--" is at col 28. + String sql = "-- first\nCREATE VIEW V AS SELECT 1; -- second\n/* third */\n"; + List cs = SqlCommentParser.parse(sql); + Assert.assertEquals(3, cs.size()); + assertComment(cs.get(0), SqlComment.Kind.LINE_DASH, "-- first\n", 1, 1); + assertComment(cs.get(1), SqlComment.Kind.LINE_DASH, "-- second\n", 2, 28); + assertComment(cs.get(2), SqlComment.Kind.BLOCK, "/* third */", 3, 1); + assertCompilesWithoutError(sql); + } + + @Test + public void testCommentParserBlockMultiline() { + // Block comment spanning two lines; position tracks correctly. + String sql = "/* line 1\n line 2 */"; + List cs = SqlCommentParser.parse(sql); + Assert.assertEquals(1, cs.size()); + assertComment(cs.get(0), SqlComment.Kind.BLOCK, sql, 1, 1); + } + + @Test + public void testCommentParserInsideSingleQuote() { + // Comment markers inside a single-quoted string are not comments. + String sql = "CREATE VIEW V AS SELECT '-- not a comment /* also not */'"; + List cs = SqlCommentParser.parse(sql); + Assert.assertEquals(0, cs.size()); + assertCompilesWithoutError(sql); + } + + @Test + public void testCommentParserInsideDoubleQuote() { + // Comment markers inside a double-quoted identifier are not comments. + String sql = + "CREATE TABLE t (\"-- not a comment\" INT);\n" + + "CREATE VIEW V AS SELECT \"-- not a comment\" FROM t;"; + List cs = SqlCommentParser.parse(sql); + Assert.assertEquals(0, cs.size()); + assertCompilesWithoutError(sql); + } + + @Test + public void testCommentParserInsideBacktick() { + // Comment markers inside a backtick-quoted identifier are not comments. + List cs = SqlCommentParser.parse("SELECT `-- not a comment` FROM t"); + Assert.assertEquals(0, cs.size()); + } + + @Test + public void testCommentParserHintNotComment() { + // Calcite query hints /*+ ... */ are not collected as comments: + // they are already in the SqlNode tree and emitted by SqlPrettyPrinter. + String sql = + "CREATE TABLE t (x INT);\n" + + "CREATE TABLE s (x INT);\n" + + "CREATE VIEW V AS SELECT /*+ broadcast(t), shard(s) */ * FROM t JOIN s USING (x);"; + List cs = SqlCommentParser.parse(sql); + Assert.assertEquals(0, cs.size()); + assertCompilesWithoutError(sql); + } + + @Test + public void testCommentParserEscapedSingleQuote() { + // '' is an escaped quote; the string continues past it, so -- is not a comment. + String sql = "CREATE VIEW V AS SELECT 'it''s -- not a comment'"; + List cs = SqlCommentParser.parse(sql); + Assert.assertEquals(0, cs.size()); + assertCompilesWithoutError(sql); + } + + @Test + public void testCommentParserEscapedDoubleQuote() { + // "" is an escaped double-quote inside a double-quoted identifier. + String sql = + "CREATE TABLE t (\"my\"\"-- not a comment\" INT);\n" + + "CREATE VIEW V AS SELECT \"my\"\"-- not a comment\" FROM t;"; + List cs = SqlCommentParser.parse(sql); + Assert.assertEquals(0, cs.size()); + assertCompilesWithoutError(sql); + } + + @Test + public void testCommentParserEscapedBacktick() { + // `` is an escaped backtick inside a backtick-quoted identifier. + List cs = SqlCommentParser.parse("SELECT `my``-- not a comment`"); + Assert.assertEquals(0, cs.size()); + } + + @Test + public void testCommentParserStringWithNewline() { + // A string literal containing a newline: the -- on line 2 is still inside the string. + List cs = SqlCommentParser.parse("SELECT 'line1\n-- not a comment\n'"); + Assert.assertEquals(0, cs.size()); + } + + @Test + public void testCommentParserCommentAfterString() { + // Real comment immediately following a closed string. + // "CREATE VIEW V AS SELECT 'value' " is 32 chars, so "--" is at col 33. + String sql = "CREATE VIEW V AS SELECT 'value' -- real comment\n"; + List cs = SqlCommentParser.parse(sql); + Assert.assertEquals(1, cs.size()); + assertComment(cs.get(0), SqlComment.Kind.LINE_DASH, "-- real comment\n", 1, 33); + assertCompilesWithoutError(sql); + } + + @Test + public void testCommentParserInsideTableDef() { + // Comments between columns of a CREATE TABLE, verifying line/col positions. + String sql = + "CREATE TABLE t (\n" + + " -- col a\n" + + " a INT,\n" + + " -- col b\n" + + " b VARCHAR\n" + + ");\n"; + List cs = SqlCommentParser.parse(sql); + Assert.assertEquals(2, cs.size()); + assertComment(cs.get(0), SqlComment.Kind.LINE_DASH, "-- col a\n", 2, 5); + assertComment(cs.get(1), SqlComment.Kind.LINE_DASH, "-- col b\n", 4, 5); + assertCompilesWithoutError(sql); + } + + @Test + public void testFormat() throws IOException, SQLException { + File input = createInputScript(""" + -- Custom type for JSON payloads + CREATE TYPE TYP AS ("Z" INT); + CREATE FUNCTION jsonstring_as_typ(l VARCHAR) RETURNS TYP; + -- Main fact table + CREATE TABLE T( + -- primary key + x INT, + y INT LATENESS 0, + "Z" TYP ARRAY, + W STRING + ) WITH ('connectors' = '[{"name": "kafka", "url": "localhost"}]'); + CREATE TABLE orders(id INT NOT NULL, product VARCHAR, amount BIGINT, customer_x INT); + /* Aggregation view */ + CREATE VIEW V WITH ('emit_final' = 'y', 'connectors' = '[]') AS + SELECT jsonstring_as_typ(W), y, SUM(T.x) as sum, COUNT(T."Z"[1]) FROM T GROUP BY T.y, W; + CREATE VIEW v2 WITH ('connectors' = '[{"name": "http", "url": "localhost:8080"}]') AS + SELECT t.y, o.product, SUM(o.amount) AS total, + CASE WHEN SUM(o.amount) > 100 THEN 'high' ELSE 'low' END AS tier + FROM t JOIN orders AS o ON t.x = o.customer_x + WHERE o.amount > (SELECT AVG(amount) FROM orders) + GROUP BY t.y, o.product + HAVING COUNT(*) > 2;"""); + File out = File.createTempFile("out", ".sql", new File(".")); + out.deleteOnExit(); + CompilerMessages execute = CompilerMain.execute("--format", "-o", out.getPath(), input.getPath()); + Assert.assertEquals(0, execute.exitCode); + String result = Utilities.readFile(out.getAbsolutePath()); + Assert.assertEquals(""" + -- Custom type for JSON payloads + CREATE TYPE typ AS ("Z" INTEGER); + CREATE FUNCTION jsonstring_as_typ (l VARCHAR) RETURNS typ; + -- Main fact table + CREATE TABLE t ( + -- primary key + x INTEGER, + y INTEGER LATENESS 0, + "Z" typ ARRAY, + w string + ) WITH ( + 'connectors' = '[{ + "name" : "kafka", + "url" : "localhost" + }]' + ); + CREATE TABLE orders ( + id INTEGER NOT NULL, + product VARCHAR, + amount BIGINT, + customer_x INTEGER + ); + /* Aggregation view */ + CREATE VIEW v WITH ( + 'emit_final' = 'y', + 'connectors' = '[ ]' + ) AS + SELECT + jsonstring_as_typ(w), + y, + SUM(t.x) AS sum, + COUNT(t."Z"[1]) + FROM t + GROUP BY t.y, w; + CREATE VIEW v2 WITH ( + 'connectors' = '[{ + "name" : "http", + "url" : "localhost:8080" + }]' + ) AS + SELECT + t.y, + o.product, + SUM(o.amount) AS total, + CASE WHEN SUM(o.amount) > 100 THEN 'high' ELSE 'low' END AS tier + FROM t + INNER JOIN orders AS o ON t.x = o.customer_x + WHERE o.amount > ( + SELECT + AVG(amount) + FROM orders + ) + GROUP BY t.y, o.product + HAVING COUNT(*) > 2;""", result); + // Reformatted program is valid + execute = CompilerMain.execute("--noRust", "-i", out.getPath()); + Assert.assertEquals(0, execute.exitCode); + Utilities.deleteFile(input, false); + } + @Test public void testWarningsAreErrors() { this.statementsFailingInCompilation(""" diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/QATests.java b/sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/QATests.java index 4a2a95ddd64..9ba3adc942f 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/QATests.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/QATests.java @@ -91,7 +91,7 @@ static List getQATests() { } @Test - public void qaTests() throws SQLException, IOException { + public void qaTests() throws SQLException { // BaseSQLTests.showPlan(); for (File c : getQATests()) { // This program cannot be compiled because it contains a udf diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/mysql/TimestampDiffTests.java b/sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/mysql/TimestampDiffTests.java index 6423ba0a4ef..6f95535d7d1 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/mysql/TimestampDiffTests.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/mysql/TimestampDiffTests.java @@ -150,6 +150,8 @@ public void testDateDiff() { ----- 89 (1 row)"""); + this.statementsFailingInCompilation("CREATE VIEW V AS SELECT DATEDIFF(MINUTE, '', '');", + "Use TIMESTAMPDIFF instead of DATEDIFF for MINUTE"); } @Test diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/simple/Regression3Tests.java b/sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/simple/Regression3Tests.java index 1c09deec61e..53c5bd5554c 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/simple/Regression3Tests.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/simple/Regression3Tests.java @@ -7,7 +7,6 @@ import org.dbsp.sqlCompiler.compiler.sql.tools.SqlIoTest; import org.dbsp.sqlCompiler.compiler.visitors.outer.CircuitVisitor; import org.junit.Assert; -import org.junit.Ignore; import org.junit.Test; public class Regression3Tests extends SqlIoTest { @@ -717,4 +716,16 @@ r ROW (b VARCHAR) CREATE VIEW Z AS SELECT b, r.b, t.b FROM T;"""); } + + @Test + public void testEnforcePositiveInputs() { + String sql = """ + SET ENFORCE_POSITIVE_INPUTS = TRUE; + CREATE TABLE T(x INT NOT NULL); + CREATE VIEW V AS SELECT x FROM T;"""; + DBSPCompiler compiler = this.testCompiler(); + compiler.submitStatementsForCompilation(sql); + this.runtimeFail(compiler, "REMOVE FROM T VALUES (1);", + "Table t: negative weight found"); + } } diff --git a/sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/tools/BaseSQLTests.java b/sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/tools/BaseSQLTests.java index 4b57fffb5e5..0d001d9dc22 100644 --- a/sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/tools/BaseSQLTests.java +++ b/sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/tools/BaseSQLTests.java @@ -446,6 +446,17 @@ protected void runtimeFail(String query, String message, InputOutputChangeStream new CompilerCircuitStream(compiler, data, this, message); } + /** Run a test that fails at runtime with a panic matching {@code message}. + * + * @param compiler compiler + * @param script DML statements (INSERT/REMOVE) that trigger the panic + * @param message expected substring of the panic message */ + protected void runtimeFail(DBSPCompiler compiler, String script, String message) { + CompilerCircuitStream ccs = new CompilerCircuitStream(compiler, this, message); + Change input = ccs.toChange(script); + ccs.addPair(input, new Change()); + } + /** Run a test that fails at runtime without needing any inputs */ protected void runtimeConstantFail(String query, String message) { this.runtimeFail(query, message, this.streamWithEmptyChanges()); diff --git a/sql-to-dbsp-compiler/calcite_version.env b/sql-to-dbsp-compiler/calcite_version.env index cac5a3695d6..062f571c5a4 100644 --- a/sql-to-dbsp-compiler/calcite_version.env +++ b/sql-to-dbsp-compiler/calcite_version.env @@ -3,5 +3,5 @@ CALCITE_REPO="https://github.com/apache/calcite.git" #CALCITE_REPO="https://github.com/mihaibudiu/calcite" CALCITE_BRANCH="main" CALCITE_CURRENT="1.42.0" -CALCITE_NEXT_COMMIT="1317946ea1c318ca7cc895031ce8ca48777fcf2d" +CALCITE_NEXT_COMMIT="7311760481af747175d0119ebdb83d58c14fc684" CALCITE_NEXT="1.43.0" diff --git a/sql-to-dbsp-compiler/using.md b/sql-to-dbsp-compiler/using.md index 3f2b2ea4014..bca7b7ca8cb 100644 --- a/sql-to-dbsp-compiler/using.md +++ b/sql-to-dbsp-compiler/using.md @@ -64,6 +64,9 @@ Usage: sql-to-dbsp [options] Input file to compile --errors Error output file; stderr if not specified Default: + --format + Output the SQL program reformatted + Default: false --handles Use handles (true) or Catalog (false) in the emitted Rust code Default: false