-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix: Parametrized bigdecimal mapping was being concatenated with params #7584
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -620,6 +620,9 @@ class Generator: | |
|
|
||
| UNSUPPORTED_TYPES: t.ClassVar[set[exp.DType]] = set() | ||
|
|
||
| TYPE_DEFAULT_PARAMS: t.ClassVar[dict[exp.DType, tuple[int, ...]]] = {} | ||
| TYPE_PARAM_BOUNDS: t.ClassVar[dict[exp.DType, tuple[int | None, ...]]] = {} | ||
|
|
||
| TIME_PART_SINGULARS: t.ClassVar = { | ||
| "MICROSECONDS": "MICROSECOND", | ||
| "SECONDS": "SECOND", | ||
|
|
@@ -1629,11 +1632,65 @@ def datatypeparam_sql(self, expression: exp.DataTypeParam) -> str: | |
| specifier = f" {specifier}" if specifier and self.DATA_TYPE_SPECIFIERS_ALLOWED else "" | ||
| return f"{this}{specifier}" | ||
|
|
||
| def datatype_param_bound_limiter( | ||
| self, expression: exp.DataType, type_value: exp.DType | ||
| ) -> exp.DataType: | ||
| params = expression.expressions | ||
|
|
||
| if not params: | ||
| if defaults := self.TYPE_DEFAULT_PARAMS.get(type_value): | ||
| expression = expression.copy() | ||
| expression.set( | ||
| "expressions", | ||
| [exp.DataTypeParam(this=exp.Literal.number(d)) for d in defaults], | ||
| ) | ||
| return expression | ||
|
|
||
| bounds = self.TYPE_PARAM_BOUNDS.get(type_value) | ||
| if not bounds: | ||
| return expression | ||
|
|
||
| new_params = list(params) | ||
| changed = False | ||
| for i, param in enumerate(params): | ||
| bound = bounds[i] if i < len(bounds) else None | ||
| if bound is None: | ||
| continue | ||
|
|
||
| param_value = param.this if isinstance(param, exp.DataTypeParam) else param | ||
| if ( | ||
| isinstance(param_value, exp.Literal) | ||
| and param_value.is_number | ||
| and int(param_value.to_py()) > bound | ||
| ): | ||
| self.unsupported( | ||
| f"{type_value.value} parameter ({int(param_value.to_py())}) " | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to convert to |
||
| f"exceeds {self.dialect.__class__.__name__}'s maximum capping to {bound}" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this reads well, did you mean to write it as |
||
| ) | ||
| new_param = param.copy() | ||
| capped = exp.Literal.number(bound) | ||
| if isinstance(new_param, exp.DataTypeParam): | ||
| new_param.set("this", capped) | ||
| else: | ||
| new_param = capped | ||
| new_params[i] = new_param | ||
| changed = True | ||
|
Comment on lines
+1670
to
+1677
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need the copy here and the swap logic? |
||
|
|
||
| if changed: | ||
| expression = expression.copy() | ||
| expression.set("expressions", new_params) | ||
| return expression | ||
|
|
||
| def datatype_sql(self, expression: exp.DataType) -> str: | ||
| nested = "" | ||
| values = "" | ||
|
|
||
| expr_nested = expression.args.get("nested") | ||
| type_value = expression.this | ||
|
|
||
| if not expr_nested and isinstance(type_value, exp.DType): | ||
| expression = self.datatype_param_bound_limiter(expression, type_value) | ||
|
Comment on lines
+1691
to
+1692
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| interior = ( | ||
| self.expressions( | ||
| expression, dynamic=True, new_line=True, skip_first=True, skip_last=True | ||
|
|
@@ -1642,7 +1699,6 @@ def datatype_sql(self, expression: exp.DataType) -> str: | |
| else self.expressions(expression, flat=True) | ||
| ) | ||
|
|
||
| type_value = expression.this | ||
| if type_value in self.UNSUPPORTED_TYPES: | ||
| self.unsupported( | ||
| f"Data type {type_value.value} is not supported when targeting {self.dialect.__class__.__name__}" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1342,7 +1342,6 @@ def test_update_positions_empty_meta(self): | |
| assert expr1.meta == {} | ||
|
|
||
| def test_pipe_and_apply(self) -> None: | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Stray diff? |
||
| def add_val(expr: exp.Expr, val: int, *, squared: bool) -> exp.Expr: | ||
| nb = val**2 if squared else val | ||
| return expr + nb | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether these could be a single data structure to avoid separate dict lookups