Skip to content

fix: Support Union and Optional in DataType.infer_from_type#6304

Open
srilman wants to merge 1 commit intomainfrom
slade/cleanup-typing
Open

fix: Support Union and Optional in DataType.infer_from_type#6304
srilman wants to merge 1 commit intomainfrom
slade/cleanup-typing

Conversation

@srilman
Copy link
Contributor

@srilman srilman commented Feb 27, 2026

Changes Made

Noticed it when I was writing some other test, and got really annoying so I just dealt with it.

Note that the workarounds seem kind of bad, so I might take a better look at it at another point. Just a temp solution.

- "metadata" or "wat": Metadata about crawled pages
+ "raw" or "warc": Raw WARC files containing full HTTP responses
+ "text" or "wet": Extracted plain text content
+ "metadata" or "wat": Metadata about crawled pages
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that in VSCode rendering, it is happier with + signs then - or *. All three are valid in Markdown, and others like IPython don't care. So I vote for using these.

(list[list], dt.list(dt.list(dt.python()))),
(list[list[str]], dt.list(dt.list(dt.string()))),
(
pytest.param(type(None), dt.null(), id="null"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added labels so its easier to tell which one fails

@srilman srilman requested a review from colin-ho February 27, 2026 00:25
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 27, 2026

Greptile Summary

This PR adds support for Union and Optional types in DataType.infer_from_type, which previously couldn't handle these type annotations.

Implementation approach:

  • Added handling for both old-style (Union[str, int]) and new-style (str | int) union syntax
  • For unions where all types are the same, returns that single type
  • For Optional types (str | None), returns the non-null type
  • For unions with multiple distinct types, falls back to python() type
  • Moved typing import to module level and added UnionType from types module

Test coverage:
Tests were added for: str | int, str | None, str | int | None, and Optional[str]. The test file was also improved by converting all test cases to use pytest.param with descriptive IDs for better test output readability.

Confidence Score: 4/5

  • Safe to merge - adds Union/Optional type support with correct logic and comprehensive test coverage
  • The implementation correctly handles Union and Optional types through recursive type inference. While the author notes this is a temporary solution, the logic is sound: unions with identical types return that type, Optional types (Type | None) return the non-null type, and other unions fall back to python(). Test coverage is comprehensive with clear test case naming.
  • No files require special attention - all changes are straightforward and well-tested

Important Files Changed

Filename Overview
daft/datatype.py Adds Union and Optional type support to type inference by checking for typing.Union or UnionType, recursively inferring member types, and handling Optional (Type
tests/test_datatype_inference.py Adds comprehensive test coverage for Union and Optional types including str

Last reviewed commit: 7b35a0f

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@srilman srilman changed the title fix: Support Union and Optional in DataType.infer_from_type. fix: Support Union and Optional in DataType.infer_from_type.= Feb 27, 2026
@srilman srilman changed the title fix: Support Union and Optional in DataType.infer_from_type.= fix: Support Union and Optional in DataType.infer_from_type\ Feb 27, 2026
@srilman srilman changed the title fix: Support Union and Optional in DataType.infer_from_type\ fix: Support Union and Optional in DataType.infer_from_type Feb 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant