-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Convert to using mashumaro jsonschema with acceptable performance #8437
Changes from all commits
e5a331d
0d0f7d9
93eb22e
dde27b6
e6d02a9
1a5f65b
f5b22cb
5149d59
68a2f96
2161503
6812aa5
6122517
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 |
---|---|---|
@@ -0,0 +1,6 @@ | ||
kind: Under the Hood | ||
body: Switch from hologram to mashumaro jsonschema | ||
time: 2023-07-18T14:54:28.41453-04:00 | ||
custom: | ||
Author: gshank | ||
Issue: "8426" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,11 +2,11 @@ | |
from enum import Enum | ||
from itertools import chain | ||
from typing import Any, List, Optional, Dict, Union, Type, TypeVar, Callable | ||
from typing_extensions import Annotated | ||
|
||
from dbt.dataclass_schema import ( | ||
dbtClassMixin, | ||
ValidationError, | ||
register_pattern, | ||
StrEnum, | ||
) | ||
from dbt.contracts.graph.unparsed import AdditionalPropertiesAllowed, Docs | ||
|
@@ -15,6 +15,7 @@ | |
from dbt.exceptions import DbtInternalError, CompilationError | ||
from dbt import hooks | ||
from dbt.node_types import NodeType | ||
from mashumaro.jsonschema.annotations import Pattern | ||
|
||
|
||
M = TypeVar("M", bound="Metadata") | ||
|
@@ -188,9 +189,6 @@ class Severity(str): | |
pass | ||
|
||
|
||
register_pattern(Severity, insensitive_patterns("warn", "error")) | ||
|
||
|
||
class OnConfigurationChangeOption(StrEnum): | ||
Apply = "apply" | ||
Continue = "continue" | ||
|
@@ -376,15 +374,6 @@ def finalize_and_validate(self: T) -> T: | |
self.validate(dct) | ||
return self.from_dict(dct) | ||
|
||
def replace(self, **kwargs): | ||
gshank marked this conversation as resolved.
Show resolved
Hide resolved
|
||
dct = self.to_dict(omit_none=True) | ||
|
||
mapping = self.field_mapping() | ||
for key, value in kwargs.items(): | ||
new_key = mapping.get(key, key) | ||
dct[new_key] = value | ||
return self.from_dict(dct) | ||
|
||
|
||
@dataclass | ||
class SemanticModelConfig(BaseConfig): | ||
|
@@ -447,11 +436,11 @@ class NodeConfig(NodeAndTestConfig): | |
persist_docs: Dict[str, Any] = field(default_factory=dict) | ||
post_hook: List[Hook] = field( | ||
default_factory=list, | ||
metadata=MergeBehavior.Append.meta(), | ||
metadata={"merge": MergeBehavior.Append, "alias": "post-hook"}, | ||
) | ||
pre_hook: List[Hook] = field( | ||
default_factory=list, | ||
metadata=MergeBehavior.Append.meta(), | ||
metadata={"merge": MergeBehavior.Append, "alias": "pre-hook"}, | ||
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. Why are aliases needed for Append now? Why does packages not need it on line 466? 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. The "alias" is for handling the dashes in the names properly. Most of the other field definitions use that kind of hacky metadata=MergeBehavior.DictKeyAppend.meta() thing, which doesn't allow setting additional metadata. |
||
) | ||
quoting: Dict[str, Any] = field( | ||
default_factory=dict, | ||
|
@@ -511,30 +500,11 @@ def __post_init__(self): | |
@classmethod | ||
def __pre_deserialize__(cls, data): | ||
data = super().__pre_deserialize__(data) | ||
field_map = {"post-hook": "post_hook", "pre-hook": "pre_hook"} | ||
# create a new dict because otherwise it gets overwritten in | ||
# tests | ||
new_dict = {} | ||
for key in data: | ||
new_dict[key] = data[key] | ||
data = new_dict | ||
for key in hooks.ModelHookType: | ||
if key in data: | ||
data[key] = [hooks.get_hook_dict(h) for h in data[key]] | ||
for field_name in field_map: | ||
if field_name in data: | ||
new_name = field_map[field_name] | ||
data[new_name] = data.pop(field_name) | ||
return data | ||
|
||
def __post_serialize__(self, dct): | ||
dct = super().__post_serialize__(dct) | ||
field_map = {"post_hook": "post-hook", "pre_hook": "pre-hook"} | ||
for field_name in field_map: | ||
if field_name in dct: | ||
dct[field_map[field_name]] = dct.pop(field_name) | ||
return dct | ||
|
||
# this is still used by jsonschema validation | ||
@classmethod | ||
def field_mapping(cls): | ||
|
@@ -554,6 +524,9 @@ def validate(cls, data): | |
raise ValidationError("A seed must have a materialized value of 'seed'") | ||
|
||
|
||
SEVERITY_PATTERN = r"^([Ww][Aa][Rr][Nn]|[Ee][Rr][Rr][Oo][Rr])$" | ||
|
||
|
||
@dataclass | ||
class TestConfig(NodeAndTestConfig): | ||
__test__ = False | ||
|
@@ -564,7 +537,8 @@ class TestConfig(NodeAndTestConfig): | |
metadata=CompareBehavior.Exclude.meta(), | ||
) | ||
materialized: str = "test" | ||
severity: Severity = Severity("ERROR") | ||
# Annotated is used by mashumaro for jsonschema generation | ||
severity: Annotated[Severity, Pattern(SEVERITY_PATTERN)] = Severity("ERROR") | ||
store_failures: Optional[bool] = None | ||
where: Optional[str] = None | ||
limit: Optional[int] = None | ||
|
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.
If this is not the intended input format, should we raise a warning here indicating that? I wouldn't cause anything to fail, but providing some direction would make it easier for us to deprecate the incorrect spelling in the future (likely one less thing for folks to change for 2.0).
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.
That ticket specifically allowed the "incorrect" spellings, so it's now a feature.
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.
There's no :lolsob: emoji, why is there no :lolsob: emoji when I need one so badly.
That being said, we don't intend on ever migrating folks off of the "incorrect" spelling either?
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.
You'd have to ask product and Doug :). If you want to open a ticket, go ahead. Not in scope for this one though...
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.
The misspelling here we mean is, we'll accept either kebab case or snake case for these two configs, in the several places they could be potentially defined:
post-hook
orpost_hook
pre-hook
orpre_hook
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.
Agreed, I'm asking if we ever want to back out of that ditch, or support that for the foreseeable future.