feat(starrocks): add partition by range and unique key

[pickfire]

No description provided.

[georgesittas]

Thanks for the PR @pickfire, please share documentation about the added features.

[georgesittas]

Let's avoid adding the new keep_number kwarg here. I tested INTERVAL '1' DAY against starrocks and even though it accepts it in expression like DATE_ADD, it doesn't accept it in the DDLs relevant to this PR. However, we can still output a number at generation time, so that would be preferable to adding complexity in the parser.

[pickfire]

INTERVAL '1' DAY gets an error in starrocks, that's why I did it this way, so what you mean is during output we tweak it to number?

[georgesittas]

Yep

[georgesittas]

It could be worth consolidating this and _parse_duplicate into a single definition that gets a Type[Expression] and instantiates it according to this (common) logic.

[pickfire]

I don't get what do you want me to do here.

[georgesittas]

Something like:

def _parse_composite_key_property(self, expr_type: t.Type[E]) -> E:
    self._match_text_seq("KEY")
    expressions = self._parse_wrapped_id_vars()
    return self.expression(expr_type, expressions=expressions

So we can use it for both _parse_unique_property and _parse_duplicate in the base parser.

[pickfire]

Fixed.

[georgesittas]

Thanks

[georgesittas]

Just double-checking: there's also this in starrocks' docs:

PARTITION <partition1_name> VALUES LESS THAN ("<upper_bound_for_partitioning_column1>" [ , "<upper_bound_for_partitioning_column2>", ... ] )

From what I understand, your changes only add support for the (START...END...EVERY...) syntax.

1. What happens today if the user tries to parse the alternative syntax?
2. Same question as (1), but given the changes in your PR?

I wanna ensure there are no regressions.

[pickfire]

I did not support this as I don't have the need to use it, other people that need it can add it later, later they can just add another expression for this, since it is totally different.

Yup, it only supports the dynamic partitioning range method (START...END...EVERY...) syntax like you mentioned.

[georgesittas]

This makes sense, but there's some nuance here. Some unsupported DDLs are still "parsed" today, meaning that we produce a Command node and store the input as raw text in it. The motivation behind this is to avoid crashing in cases where transpilation / parsing is not required for certain statements.

Since you introduced new logic for this property, I wanted to ensure that we don't break these Command statements accidentally by entering this if self._match_text_seq("RANGE") branch.

Basically let's test what happened before vs after. If the VALUES LESS THAN clause was not supported before, it's fine. If it was parsed into a Command, we need to make sure that if the parser fails, we'll fallback to a command as well.

Does this make sense?

[pickfire]

I think I will ignore this for now, don't want to spend too much time on this.

[georgesittas]

Thanks for the PR.