Document SQL dialect guidance #13706

alamb · 2024-12-09T15:07:07Z

Which issue does this PR close?

Closes Document the SQL dialect DataFusion attempts to follow #13704

Rationale for this change

Determining what is the "correct behavior" comes frequently deciding how a function should behave and there is disagreement across implementations (e.g. postgres does something different than spark and/or duckdb)

This happened most recently here with @comphead and @jayzhan211 and @Kimahriman in #13683 (comment)

What changes are included in this PR?

Try and document what the current behavior is (please double check me here)

Are these changes tested?

By CI

Are there any user-facing changes?

New docs page.

In my mind this doesn't change any semantics or policy, it simply documents the current reality. However, I am not 100% everyone has the same understanding of the current reality 🤔

Kimahriman · 2024-12-09T15:29:31Z

docs/source/user-guide/sql/dialect.md

+By default, DataFusion follows the [PostgreSQL SQL dialect].
+For Array/List functions and semantics, it follows the [DuckDB SQL dialect].


Is it confusing when an array/list function exists in PostgreSQL but you don't follow it's semantics, even though you do for non array/list functions?

I think it is confusing for sure

docs/source/user-guide/sql/dialect.md

comphead

its good to get this documented
Thanks @alamb

alamb · 2024-12-09T20:07:15Z

I plan to leave this open for several days to allow additional time for reviews / comments

findepi · 2024-12-10T10:50:04Z

docs/source/user-guide/sql/dialect.md

+semantics as well as documenting and teaching users about them.
+
+As Apache DataFusion is highly customizable, systems built on DataFusion can and do
+update functions and SQL syntax to model other systems, such as Apache Spark or


and type rules

findepi · 2024-12-10T10:51:13Z

docs/source/user-guide/sql/dialect.md

+
+# SQL Dialect
+
+By default, Apache DataFusion follows the [PostgreSQL SQL dialect].


When reading something that includes "by default", a reader may have natural question "how do i change this?"

Should we have a tip how to amend the behavior if the default doesn't please the user?

Good idea -- I tried to do so in 8fd7f44

findepi · 2024-12-10T10:54:55Z

docs/source/user-guide/sql/dialect.md

+  under the License.
+-->
+
+# SQL Dialect


it would be good to say this section pertains the SQL frontend:

sql parser and sql-to-rel

dataframe API

analyzer and the type coercions done at this stage

function semantics of functions bundled with datafusion

see also #13704 (comment)

it's also worth noting that we are not going to align with PostgreSQL's dialect fully

eg our type system is different (inherited from arrow)

we provide extended syntax for certain operations (like CREATE TABLE)

(...)

Reworded and expanded in b674a42

findepi · 2024-12-10T11:06:50Z

docs/source/user-guide/sql/dialect.md

+[duckdb sql dialect]: https://duckdb.org/docs/sql/functions/array
+[postgresql sql dialect]: https://www.postgresql.org/docs/current/sql.html


Is this dualism good for users?

i think the market formed consensus to regard PostgreSQL-compatible as a good thing. Why do we water this down to say PostgreSQL-compatible, except where not? It's going to be confusing and incoherent

for example these expressions should be equivalent

2 IN (1, NULL, 3)

2 in (SELECT * FROM (VALUES (1), (NULL), (3)))

is 2 in [1, NULL, 3] array

whereas we say that first two are governed by "be like PostgreSQL" goal
and the last is governed by "be like DuckDB" goal

Why would users appreciate that?

I don't know how good this is for users -- I think this choice (to deviate from postgres and instead follow DuckDB) was driven by the desires of the original implementers. Maybe @jayzhan211 remembers more of the rationale

Nested functions are not well-supported in Postgres, so we choose DuckDB and other OLAP systems for these kind of function. We don’t enforce strict adherence to any particular system for function implementation. Instead, our goal is to establish a default behavior in the DataFusion core while making it easy to customize functions as needed. Given that Postgres and DuckDB are enough for us to decide what the function should be like so we ends up these 2.

Instead of focusing on which system we should be compatible with, our goal should be to make it easy to customize for different systems. For example, Spark and Postgres handle nulls differently, as does Datafusion. However, we currently lack a straightforward way to adjust null handling. Users are forced to copy entire functions and maintain them independently, which isn't ideal.

#6855 is the original discussion about switching to other OLAP system for nested function

But we need to admit the fact that we cannot just say "we follow dialect/database X, but for subcomponent we follow dialect/database Y", as this leads to inconsistencies.

I think the "inconsistency" mentioned here is what we want, learn the lesson from existing systems and choose the "better" one among them. The downside of "inconsistency" can be minimize if we document the rationale of the decision and the difference from other systems.

What I am trying to do with this PR is to provide practical guidance about what to do when we we hit a question like in #13683 (comment) (what should the array_has behavior be in the presence of nulls?)

I think it we could just say "do whatever DuckDB does in this case" that would be much more efficent and the PR would not get stuck

I realize there may not be complete alignment on how DataFusion's SQL should behave and I am in full support of making it as configurable as possible

I think there is a tension between

Following an existing non ideal dialect (all existing dialects are non ideal) - which is predictable, and easier (we don't have to figure out semantics)

Creating a new dialect that we feel is better - but now users have to learn that dialect and we have to define exactly what it means

Creating a new dialect that we feel is better - but now users have to learn that dialect and we have to define exactly what it means

I also would prefer to avoid doing that.
But in fact we're doing exactly that. It's a stretch to pretend we're not shipping "DataFusion's dialect of SQL".

I think it we could just say "do whatever DuckDB does in this case" that would be much more efficent and the PR would not get stuck

Yes, that's efficient.
But same efficiency you get from "do whatever PostgreSQL does" (efficiency in terms of answering on-PR design questions)
And same efficiency you get from "do whatever Trino does"

We're trading answer efficiency vs consistency and predictability of our of the box semantics, with both sides of the trade being desirable properties.

It's likely that maximum consistency you get from "do whatever SQL spec says", but obviously this isn't very efficient (most engineers can't even read the spec).

Following an existing non ideal dialect (all existing dialects are non ideal) - which is predictable, and easier (we don't have to figure out semantics)

That would be great, especially if this a reasonable dialect (some are, some are not) and is one dialect, not some form of a synergy between two different dialects that disagree with each other.

Maybe we can change this section to be titles "Aspirational SQL dialect" or something.

It's a stretch to pretend we're not shipping "DataFusion's dialect of SQL".

I fully admit that what is supported by DataFusion is not exactly the same as any existing dialect. Maybe it is wishful thinking that we are "close enough" to postgres and that defining the goal (even if we never reach it) is valuable 🤔

defining the goal (even if we never reach it) is valuable 🤔

Agreed!

We should have a goal, and we should have a litmus test for determining what gets in and with what semantics

alamb

Thank you @findepi and @comphead for the reviews

alamb · 2024-12-10T12:03:45Z

docs/source/user-guide/sql/dialect.md

+[duckdb sql dialect]: https://duckdb.org/docs/sql/functions/array
+[postgresql sql dialect]: https://www.postgresql.org/docs/current/sql.html


I don't know how good this is for users -- I think this choice (to deviate from postgres and instead follow DuckDB) was driven by the desires of the original implementers. Maybe @jayzhan211 remembers more of the rationale

alamb · 2024-12-10T12:19:47Z

docs/source/user-guide/sql/dialect.md

+  under the License.
+-->
+
+# SQL Dialect


Reworded and expanded in b674a42

alamb · 2024-12-10T12:19:49Z

docs/source/user-guide/sql/dialect.md

+
+# SQL Dialect
+
+By default, Apache DataFusion follows the [PostgreSQL SQL dialect].


Good idea -- I tried to do so in 8fd7f44

crepererum · 2024-12-10T16:00:05Z

docs/source/user-guide/sql/dialect.md

+
+SQL Engines have a choice to either use an existing SQL dialect or define their
+own. Using an existing dialect may not fit perfectly as it is hard to match
+semantics exactly (need bug-for-bug compatibility), and is likely not what all


More reasons for using pogstgres specifically: it is

rather extensive, in contrast to e.g. sqlite

rather well aligned w/ the SQL standard (at least that's my personal impression, after having faced MySQL)

rather well aligned w/ the SQL standard (at least that's my personal impression, after having faced MySQL)

mostly true (but i know of some deviations)
if we wanted something "executable but also aligned with SQL std", I'd recommend Trino

i kind of assumed PostgreSQL ship has sailed and we're just retro-documenting. But if the ball (choice) is still in play, my vote goes to Trino as a good reference implementation.

i kind of assumed PostgreSQL ship has sailed and we're just retro-documenting.

That was my assumption too, but as others like @jayzhan211 (and yourself) have pointed out, I don't think DataFusion is (or can be) 100% consistent with this point

docs/source/user-guide/sql/dialect.md

findepi · 2024-12-11T09:13:22Z

docs/source/user-guide/sql/dialect.md

+[duckdb sql dialect]: https://duckdb.org/docs/sql/functions/array
+[postgresql sql dialect]: https://www.postgresql.org/docs/current/sql.html


I agree we shouldn't follow PostgreSQL arrays as the guide. PostgreSQL arrays are not structured. You can declare dimensions of an array column, but that's advisory only. Each value can be array of different dimensions. That won't work well with Arrow type system - there is no arrow type that corresponds to PostgreSQL array type.

But we need to admit the fact that we cannot just say "we follow dialect/database X, but for subcomponent we follow dialect/database Y", as this leads to inconsistencies.

Yes, we will address needs of downstream projects by providing necessary extension points. In the worst case, they can override "everything".
But DataFusion also has its frontend and it should strive to be consistent. Consistency is important both to end users and also to people building on top of datafusion.

findepi · 2024-12-11T09:14:26Z

docs/source/user-guide/sql/dialect.md

+
+Notable exceptions:
+
+- Array/List functions and semantics follow the [DuckDB SQL dialect].


Not quite? DuckDB array seems to be fixed size. Is this saying DF array follows DuckDB's list semantics?

yes DuckDB's "list" not "array". We prefer to convert fixed-size lists into regular lists, so we don’t make a distinction between the two

is this going to be obvious to the reader of this document?

docs/source/user-guide/sql/dialect.md

findepi · 2024-12-11T09:17:38Z

docs/source/user-guide/sql/dialect.md

+
+SQL Engines have a choice to either use an existing SQL dialect or define their
+own. Using an existing dialect may not fit perfectly as it is hard to match
+semantics exactly (need bug-for-bug compatibility), and is likely not what all


rather well aligned w/ the SQL standard (at least that's my personal impression, after having faced MySQL)

mostly true (but i know of some deviations)
if we wanted something "executable but also aligned with SQL std", I'd recommend Trino

i kind of assumed PostgreSQL ship has sailed and we're just retro-documenting. But if the ball (choice) is still in play, my vote goes to Trino as a good reference implementation.

Co-authored-by: Piotr Findeisen <[email protected]>

alamb · 2024-12-15T22:13:59Z

Update: I don't feel like we have reached consensus on this ticket yet, so I don't have any specific plans to merge it yet

liurenjie1024 · 2024-12-18T03:36:50Z

docs/source/user-guide/sql/dialect.md

+The included SQL supported in Apache DataFusion mostly follows the [PostgreSQL
+SQL dialect], including:
+
+- The SQL parser and [SQL planner]


Suggested change

- The SQL parser and [SQL planner]

- SQL Syntax

I think for user SQL syntax is more clear. Also I don't see why SQL planner is involved in sql dialect.

for user -- agreed.
but DF is also a library of reusable components.
The parser and sql-to-rel also follow some specific dialect's semantics, and this should be documented.
Not sure if it belongs under user-guide/ though.

alamb · 2024-12-19T20:13:26Z

I removed @comphead's approval so it was clear this PR is not ready to merge (from my perspective)

alamb added 2 commits December 9, 2024 10:04

Documentat SQL dialect followed

6bb2aa8

prettier

5af035c

github-actions bot added the documentation Improvements or additions to documentation label Dec 9, 2024

Kimahriman reviewed Dec 9, 2024

View reviewed changes

comphead reviewed Dec 9, 2024

View reviewed changes

docs/source/user-guide/sql/dialect.md Outdated Show resolved Hide resolved

comphead reviewed Dec 9, 2024

View reviewed changes

docs/source/user-guide/sql/dialect.md Outdated Show resolved Hide resolved

comphead reviewed Dec 9, 2024

View reviewed changes

docs/source/user-guide/sql/dialect.md Outdated Show resolved Hide resolved

Apply suggestions from code review

49edb03

comphead approved these changes Dec 9, 2024

View reviewed changes

findepi reviewed Dec 10, 2024

View reviewed changes

alamb mentioned this pull request Dec 10, 2024

Document the SQL dialect DataFusion attempts to follow #13704

Open

alamb added 2 commits December 10, 2024 07:15

try and clarify the postgres behavior refers to frontend

b674a42

clarify how to change behavior

8fd7f44

alamb commented Dec 10, 2024

View reviewed changes

crepererum reviewed Dec 10, 2024

View reviewed changes

findepi reviewed Dec 11, 2024

View reviewed changes

alamb and others added 2 commits December 11, 2024 07:39

Update docs/source/user-guide/sql/dialect.md

e9f5da1

Co-authored-by: Piotr Findeisen <[email protected]>

Apply suggestions from code review

c6a83f3

Co-authored-by: Piotr Findeisen <[email protected]>

alamb changed the title ~~Document SQL dialect~~ Document SQL dialect guidance Dec 11, 2024

Kimahriman mentioned this pull request Dec 12, 2024

feat: add support for array_contains expression apache/datafusion-comet#1163

Open

comphead mentioned this pull request Dec 13, 2024

Initcap behaves differently in Spark and in DataFusion (also Comet) apache/datafusion-comet#1052

Open

findepi mentioned this pull request Dec 13, 2024

flatten should be single-step, not recursive #13757

Open

liurenjie1024 reviewed Dec 18, 2024

View reviewed changes

alamb requested a review from comphead December 19, 2024 20:13

		By default, DataFusion follows the [PostgreSQL SQL dialect].
		For Array/List functions and semantics, it follows the [DuckDB SQL dialect].


		# SQL Dialect

		By default, Apache DataFusion follows the [PostgreSQL SQL dialect].

		[duckdb sql dialect]: https://duckdb.org/docs/sql/functions/array
		[postgresql sql dialect]: https://www.postgresql.org/docs/current/sql.html


		Notable exceptions:

		- Array/List functions and semantics follow the [DuckDB SQL dialect].

Document SQL dialect guidance #13706

Are you sure you want to change the base?

Document SQL dialect guidance #13706

Conversation

alamb commented Dec 9, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comphead left a comment

Choose a reason for hiding this comment

alamb commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Dec 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Dec 19, 2024

jayzhan211 Dec 10, 2024 •

edited

Loading