Add support for PostgreSQL/Redshift geometric operators #1723

benrsatori · 2025-02-11T08:54:53Z

Add support for PostgreSQL and Redshift geometric operators.

iffyio · 2025-02-11T15:30:44Z

src/ast/operator.rs

+    /// `+` Addition, also Translation (PostgreSQL/Redshift geometric operator), e.g. `box '((0,0),(1,1))' + point '(2.0,0)'`
    Plus,
-    /// Minus, e.g. `a - b`
+    /// `-` Subtraction, Translation (PostgreSQL/Redshift geometric operator), e.g. `box '((0,0),(1,1))' - point '(2.0,0)'`
    Minus,
-    /// Multiply, e.g. `a * b`
+    /// `*` Multiplication, also Scaling (PostgreSQL/Redshift geometric operator), e.g. `box '((0,0),(1,1))' * point '(2.0,0)'`
    Multiply,
-    /// Divide, e.g. `a / b`
+    /// `/` Division, also Scaling (PostgreSQL/Redshift geometric operator), e.g. `box '((0,0),(2,2))' / point '(2.0,0)'`


Can we skip this diff? I'm thinking since these operators are fundamental and apply to various scenarios, no need to mention specific dialect semantics given neither the parser nor AST handles them specially as a result

iffyio · 2025-02-11T15:33:05Z

src/ast/operator.rs

+    /// `#` Number of points in path or polygon (PostgreSQL/Redshift geometric operator), e.g. `# path '((1,0),(0,1),(-1,0))'`
+    NumOfPoints,
+    /// `@-@` Length or circumference (PostgreSQL/Redshift geometric operator), e.g. `@-@ path '((0,0),(1,0))'`
+    LenOfCircumference,


the doc says length or circumference, is the Of a typo?

it's a typo, fixed

iffyio · 2025-02-11T15:33:56Z

src/ast/operator.rs

@@ -53,6 +53,16 @@ pub enum UnaryOperator {
    PGAbs,
    /// Unary logical not operator: e.g. `! false` (Hive-specific)
    BangNot,
+    /// `#` Number of points in path or polygon (PostgreSQL/Redshift geometric operator), e.g. `# path '((1,0),(0,1),(-1,0))'`


Could we include a link to the docs where the operators are described? that would be helpful for folks that come across it and need to find more context

iffyio · 2025-02-11T15:35:52Z

src/ast/operator.rs

+    // `#` PostgreSQL Bitwise XOR, also Intersection (PostgreSQL/Redshift geometric operator), e.g. `box '((1,-1),(-1,1))' # box '((1,1),(-2,-2))'`
    PGBitwiseXor,
-    /// Bitwise shift left, e.g. `a << b` (PostgreSQL-specific)
+    /// `<<` PostgreSQL Bitwise Shift Left, also Left of? (PostgreSQL/Redshift geometric operator), e.g. circle '((0,0),1)' << circle '((5,0),1)'
    PGBitwiseShiftLeft,
-    /// Bitwise shift right, e.g. `a >> b` (PostgreSQL-specific)
+    /// `>>` PostgreSQL Bitwise Shift Right, Right of? (PostgreSQL/Redshift geometric operator), e.g. circle '((5,0),1)' >> circle '((0,0),1)'


actually same with these, I think just mentioning the operator suffices, without going into semantics

iffyio · 2025-02-11T15:41:17Z

src/ast/operator.rs

@@ -253,6 +268,40 @@ pub enum BinaryOperator {
    /// Specifies a test for an overlap between two datetime periods:
    /// <https://jakewheat.github.io/sql-overview/sql-2016-foundation-grammar.html#overlaps-predicate>
    Overlaps,
+    /// `##` Point of closest proximity (PostgreSQL/Redshift geometric operator), e.g. `point '(0,0)' ## lseg '((2,0),(0,2))'`
+    PointOfClosestProximity,


hmm actually thinking about it, I feel like the naming of the operators might become problematic, e.g. if another dialect supports ## operator then calling it PointOfClosestProximity would be confusing vs if the operator was called DoubleHash dialect agnostic. Would it make sense to name the operators in such a manner? i.e. by their characters or if theres a well known description that isnt describing the pg behavior

iffyio · 2025-02-11T15:46:21Z

src/parser/mod.rs

+                        if dialect_of!(self is PostgreSqlDialect | RedshiftSqlDialect)
+                            && GEOMETRIC_TYPES.contains(&w.keyword)
+                        {
+                            self.parse_geometric_type(w.keyword)


could this be its own branch within parse_expr_prefix_by_reserved_word instead?

It looks like we should be able to do something like

kw if self.dialect.supports_pg_geometric_types() && PG_GEOMETRIC_TYPES.contains(kw) => { self.parse_geometric_expr(kw) }

iffyio · 2025-02-11T15:48:13Z

src/parser/mod.rs

+        let token_with_span = self.next_token(); // Get the full TokenWithSpan
+        let value = match token_with_span.token {
+            // Extract the Token field
+            Token::SingleQuotedString(s) => Value::SingleQuotedString(s),
+            _ => return self.expected("SingleQuotedString", token_with_span), // Pass full TokenWithSpan
+        };


Suggested change

let token_with_span = self.next_token(); // Get the full TokenWithSpan

let value = match token_with_span.token {

// Extract the Token field

Token::SingleQuotedString(s) => Value::SingleQuotedString(s),

_ => return self.expected("SingleQuotedString", token_with_span), // Pass full TokenWithSpan

};

let token_with_span = self.next_token();

let value = match token_with_span.token {

Token::SingleQuotedString(s) => Value::SingleQuotedString(s),

_ => return self.expected("SingleQuotedString", token_with_span),

};

thinking we can drop the comments in this case since the code is as descriptive

iffyio · 2025-02-11T15:57:31Z

src/parser/mod.rs

+            | tok @ Token::AtAt
+            | tok @ Token::QuestionMarkDash
+            | tok @ Token::QuestionPipe
+                if dialect_of!(self is PostgreSqlDialect | RedshiftSqlDialect) =>


Can we replace these with a dialect method? the dialect_of has been deprecated, we can introduce a method like supports_pg_geometric_types() or similar

iffyio · 2025-02-11T16:00:07Z

src/test_utils.rs

+pub fn only_psql_redshift() -> TestedDialects {
+    TestedDialects {
+        dialects: vec![
+            Box::new(PostgreSqlDialect {}),
+            Box::new(RedshiftSqlDialect {}),
+        ],
+        options: None,
+        recursion_limit: None,
+    }
+}
+


by switching to the dialect method, it would be possible to reuse all_dialects_where in place of this

I removed it

Updated the code to use all_dialects_where as to your suggestion

iffyio · 2025-02-11T16:01:59Z

tests/sqlparser_postgres.rs

+        //"&<", // (is strictly to the left of)                 // Same as OverlapsToLeft, causes duplicate failure
+        //"&>", // (is strictly to the right of)                // Same as OverlapsToRight, causes duplicate failure
        "|=|",   //  distance between A and B trajectories at their closest point of approach
        "<<#>>", // n-D distance between A and B bounding boxes
-        "|>>",   // A's bounding box is strictly above B's.
-        "~=",    // bounding box is the same
+        //"|>>",   // A's bounding box is strictly above B's.   // Same as IsStrictlyAbove, causes duplicate failure
+        //"~=", // bounding box is the same                     // Same as SameAs, causes duplicate failure


not sure I followed why these are commented out, what do we mean by causes duplicate failure?

The commented-out operators are already defined under different names in the code,
so they can no longer be considered custom operators.
If we include these again under their original symbols, it causes a duplicate match failure.
so I have now completely removed them.

iffyio

Thanks @benrsatori! Left some comments, the changes look good to me overall

iffyio · 2025-02-14T16:10:14Z

src/dialect/mod.rs

@@ -912,6 +927,10 @@ pub trait Dialect: Debug + Any {
    fn supports_array_typedef_size(&self) -> bool {
        false
    }
+    /// Returns true if the dialect supports geometric types.


Could we add a link to the postgres docs here (and maybe an example sql)? it would help folks with context on what we mean by geometric types

iffyio · 2025-02-14T16:11:41Z

src/parser/mod.rs

@@ -3079,6 +3128,55 @@ impl<'a> Parser<'a> {
            Token::QuestionAnd => Some(BinaryOperator::QuestionAnd),
            Token::QuestionPipe => Some(BinaryOperator::QuestionPipe),
            Token::CustomBinaryOperator(s) => Some(BinaryOperator::Custom(s.clone())),
+            Token::DoubleSharp if dialect_of!(self is PostgreSqlDialect | RedshiftSqlDialect) => {


for these clauses as well could we use the introduced self.dialect.supports_geometric_types()?

iffyio · 2025-02-14T16:12:52Z

src/tokenizer.rs

@@ -1308,6 +1356,28 @@ impl<'a> Tokenizer<'a> {
                                _ => self.start_binop(chars, "||", Token::StringConcat),
                            }
                        }
+                        Some('&') if dialect_of!(self is PostgreSqlDialect | RedshiftSqlDialect) => {


similarly for these clauses it would be nice to use the self.dialect.supports_geometric_types() method

iffyio · 2025-02-14T16:14:17Z

src/ast/data_type.rs

@@ -879,3 +882,29 @@ pub enum ArrayElemTypeDef {
    /// `Array(Int64)`
    Parenthesis(Box<DataType>),
 }
+
+#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]


Could we add a short description to this struct with a link to the docs?

iffyio · 2025-02-14T16:14:57Z

src/ast/operator.rs

@@ -53,6 +53,21 @@ pub enum UnaryOperator {
    PGAbs,
    /// Unary logical not operator: e.g. `! false` (Hive-specific)
    BangNot,
+    /// `#` Number of points in path or polygon (PostgreSQL/Redshift geometric operator)
+    /// see <https://www.postgresql.org/docs/9.5/functions-geometry.html>
+    NumOfPoints,


Can we do similar to the binary operator where we name them according to their symbols to be dialect agnostic?

iffyio · 2025-02-14T16:15:28Z

src/ast/data_type.rs

@@ -386,6 +386,8 @@ pub enum DataType {
    ///
    /// [bigquery]: https://cloud.google.com/bigquery/docs/user-defined-functions#templated-sql-udf-parameters
    AnyType,
+    /// geometric type


Could we include a link to the postgres docs here?

iffyio

LGTM! Thanks @benrsatori!
cc @alamb

iffyio · 2025-02-19T14:04:49Z

@benrsatori could you take a look at the CI failures?

benrsatori · 2025-02-20T13:39:12Z

Hi @iffyio
I fixed the CI

iffyio reviewed Feb 11, 2025

View reviewed changes

benrsatori force-pushed the geo-op branch 3 times, most recently from b297ccb to 01f6646 Compare February 13, 2025 11:21

iffyio reviewed Feb 14, 2025

View reviewed changes

benrsatori force-pushed the geo-op branch 2 times, most recently from 9c62ffc to fdc90cf Compare February 18, 2025 10:31

iffyio approved these changes Feb 19, 2025

View reviewed changes

Add support for PostgreSQL/Redshift geometric operators

642519d

benrsatori force-pushed the geo-op branch from 4005381 to 642519d Compare February 20, 2025 13:29

iffyio merged commit 339239d into apache:main Feb 20, 2025
9 checks passed

Add support for PostgreSQL/Redshift geometric operators #1723

Add support for PostgreSQL/Redshift geometric operators #1723

Conversation

benrsatori commented Feb 11, 2025

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

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

iffyio 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

Choose a reason for hiding this comment

iffyio left a comment

Choose a reason for hiding this comment

iffyio commented Feb 19, 2025

benrsatori commented Feb 20, 2025