Skip to content

refactor (datatypes): add from_str constructor#7465

Open
OutSquareCapital wants to merge 2 commits intotobymao:mainfrom
OutSquareCapital:datatype-constructors
Open

refactor (datatypes): add from_str constructor#7465
OutSquareCapital wants to merge 2 commits intotobymao:mainfrom
OutSquareCapital:datatype-constructors

Conversation

@OutSquareCapital
Copy link
Copy Markdown
Contributor

@OutSquareCapital OutSquareCapital commented Apr 7, 2026

This PR add a new DatatType.from_str() classmethod constructor, and modify call sites to use it when we know a str will be used.

This brings the following benefit:

  • Performance improvement, since we now avoid a redundant isinstance check
  • build is more readable
  • A few call sites were using build when DType.into_expr() could be directly used instead. Now fixed
  • Use Self for return types and cls instead of DataType (class name agnostic).

Note

I still hesitate between from_str and parse for the method name.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here we were passing a dialect, even tough kind is statically known to be a DType instance. So this parameter had no effect at runtime. better to directly use DType.into_expr()

@georgesittas
Copy link
Copy Markdown
Collaborator

Performance improvement, since we now avoid a redundant isinstance check

@OutSquareCapital did you benchmark this? Can you provide some actual numbers to understand the impact of this change? Cover both paths where the inputs are strings and paths where inputs are non-strings to measure the impact.

raise
return cls.from_str(dtype, dialect, udt, **kwargs)
elif isinstance(dtype, DType):
data_type_exp = DataType(this=dtype)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why was this section until L373 refactored?

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.

2 participants