comphead commented on code in PR #57: URL: https://github.com/apache/datafusion-spark/pull/57#discussion_r4566025629
########## crates/spark/src/lib.rs: ########## @@ -11,16 +11,61 @@ #![deny(clippy::clone_on_ref_ptr)] #![cfg_attr(test, allow(clippy::needless_pass_by_value))] -//! Spark Expression packages for [DataFusion]. +//! [Apache Spark]-compatible expressions for [Apache DataFusion]. //! -//! This crate contains a collection of various Spark function packages for DataFusion, -//! implemented using the extension API. +//! DataFusion's built-in functions follow PostgreSQL / DuckDB conventions. When +//! the same function name exists in Apache Spark, the behavior often differs: +//! NULL handling, integer overflow, string indexing (1-based, negative +//! indices), rounding modes, date/time handling, error behavior in ANSI and +//! non-ANSI mode, and so on. This crate provides implementations of Spark +//! functions that follow Spark's semantics, so that engines built on DataFusion +//! (for example [Apache DataFusion Comet]) and applications migrating SQL from +//! Spark get the results Spark would produce. //! -//! [DataFusion]: https://crates.io/crates/datafusion +//! The crate depends only on the DataFusion building blocks (`datafusion-expr`, +//! `datafusion-common`, ...), not on the full `datafusion` crate, unless the +//! optional `core` feature is enabled. //! +//! # Getting started +//! +//! ```toml +//! [dependencies] +//! datafusion = "X" +//! # `core` enables `SessionStateBuilderSpark`, the easiest way to turn Spark mode on +//! datafusion-spark = { version = "X", features = ["core"] } Review Comment: This `core` snippet is repeated under "Example: enabling Apache Spark features with SessionStateBuilder" (lines 155-157). Could we keep one copy and link to it from the other section, so the two do not drift? ########## crates/spark/src/function/predicate/mod.rs: ########## @@ -2,6 +2,8 @@ // SPDX-FileCopyrightText: See the NOTICE file distributed with this work for additional information regarding copyright ownership // SPDX-FileContributor: Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements +//! Spark predicate functions. No functions are implemented in this package yet. Review Comment: "No functions are implemented in this package yet" appears in seven packages (`csv`, `lambda`, `predicate`, `struct`, `table`, `window`, `xml`), and nothing prompts an update when the first function lands. For example apache/datafusion#27078 adds `at_least_n_non_nulls` to `predicate`. Could these say only what the package is for, for example `//! Spark predicate functions.`? ########## crates/spark/src/lib.rs: ########## @@ -11,16 +11,61 @@ #![deny(clippy::clone_on_ref_ptr)] #![cfg_attr(test, allow(clippy::needless_pass_by_value))] -//! Spark Expression packages for [DataFusion]. +//! [Apache Spark]-compatible expressions for [Apache DataFusion]. //! -//! This crate contains a collection of various Spark function packages for DataFusion, -//! implemented using the extension API. +//! DataFusion's built-in functions follow PostgreSQL / DuckDB conventions. When +//! the same function name exists in Apache Spark, the behavior often differs: +//! NULL handling, integer overflow, string indexing (1-based, negative +//! indices), rounding modes, date/time handling, error behavior in ANSI and +//! non-ANSI mode, and so on. This crate provides implementations of Spark +//! functions that follow Spark's semantics, so that engines built on DataFusion +//! (for example [Apache DataFusion Comet]) and applications migrating SQL from +//! Spark get the results Spark would produce. //! -//! [DataFusion]: https://crates.io/crates/datafusion +//! The crate depends only on the DataFusion building blocks (`datafusion-expr`, +//! `datafusion-common`, ...), not on the full `datafusion` crate, unless the +//! optional `core` feature is enabled. //! +//! # Getting started +//! +//! ```toml +//! [dependencies] +//! datafusion = "X" +//! # `core` enables `SessionStateBuilderSpark`, the easiest way to turn Spark mode on +//! datafusion-spark = { version = "X", features = ["core"] } +//! ``` //! //! # Available Function Packages -//! See the list of [modules](#modules) in this crate for available packages. +//! +//! Functions are grouped into packages by category (for example [`function::string`], +//! [`function::datetime`], [`function::math`], [`function::hash`]). See the list of +//! [modules](#modules) in this crate for the available packages. Each package +//! provides: +//! +//! * a `functions()` method returning all of its UDFs for registration +//! * an `expr_fn` module with helpers to build [`Expr`]s from Rust +//! * one accessor per function (for example `function::hash::sha2()`) returning the UDF +//! +//! Not every Spark function is implemented yet; coverage is growing. The +//! [`datafusion-spark` epic] tracks progress. +//! +//! # Semantics and caveats +//! +//! * **Registration overrides built-ins.** Spark functions are registered under +//! their Spark names. If DataFusion already has a function with that name, +//! the Spark one replaces it for the session. Register Spark functions after +//! DataFusion's defaults, and only when you want Spark behavior. Review Comment: This bullet says the same thing as the `register_all` paragraph below ("Any existing functions will be overwritten, with these Spark functions taking priority"). Could we keep it in one place? This section is a good home, and the example can then link to it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
