Skip to content

Conversation

@surister
Copy link
Member

@surister surister commented Jun 11, 2024

Summary of the changes / Why this is an improvement

This PR allows for extracting properties defined by [ WITH ( parameter [= value] [, ... ] ) ] statements.

For example:

from cratedb_sqlparse import sqlparse

stmt = sqlparse("""
    CREATE TABLE doc.tbl12 (A TEXT) WITH (
      "allocation.max_retries" = 5,
      "blocks.metadata" = false
    );
""")[0]

print(stmt.metadata)
# Metadata(schema='doc', table_name='tbl12', interpolated_properties={}, with_properties={'allocation.max_retries': '5', 'blocks.metadata': 'false'})

It also handles the special case of what I call, interpolated values and #31 if you have a better name recommendation, please go ahead :P

Checklist

  • Link to issue this PR refers to (if applicable):
  • CLA is signed

@surister surister force-pushed the with_properties branch 2 times, most recently from 22d33c7 to 34ad12c Compare June 13, 2024 15:29
@surister surister marked this pull request as ready for review June 13, 2024 15:31
@surister surister requested review from amotl and matriv June 13, 2024 15:31
# Metadata(schema='doc', table_name='tbl12', interpolated_properties={}, with_properties={'allocation.max_retries': '5', 'blocks.metadata': 'false'})
```

#### Interpolated properties.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get what's the value of splitting those into an extra key in metadata.
Could you please explain what is the thought behind this?

(If we keep it, for the name I think Query parameterized properties sounds a bit better)

Copy link
Member Author

@surister surister Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just conveniency, so users don't need to constantly filter properties to get those that are parametrized, without this I see myself (as an user) extending the API to filter this parameters.

I like parameterized properties. 👍

Comment on lines 33 to +35
def visitTableName(self, ctx: SqlBaseParser.TableNameContext):
fqn = ctx.qname()
parts = self.get_text(fqn).replace('"', "").split(".")
parts = self.get_text(fqn).split(".")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# Metadata(schema='doc', table_name='tbl12', interpolated_properties={'blocks.metadata': '$1'}, with_properties={'allocation.max_retries': '5', 'blocks.metadata': '$1'})
```

In this case, both `blocks.metadata` will be in `with_properties` and `interpolated_properties`.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
In this case, both `blocks.metadata` will be in `with_properties` and `interpolated_properties`.
In this case, `blocks.metadata` will be in `with_properties` and `interpolated_properties` as well.

@surister surister merged commit 373d682 into crate:main Jun 14, 2024
assert all(x in stmt.metadata.parameterized_properties for x in keys)
assert all(x in stmt.metadata.with_properties for x in keys)
assert stmt.metadata.with_properties["key"] == "$1"
assert stmt.metadata.with_properties["key2"] == "$2"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we also assert the parameterized_properties here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌 Not only that but we can just do

    expected = {'key': 'val', 'key2': '2'}

    # Has all the keys.
    assert stmt.metadata.with_properties == expected

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants