Skip to content

Add defaults to ViewVersion fields#3458

Open
ebyhr wants to merge 1 commit into
apache:mainfrom
ebyhr:ebi/view-default
Open

Add defaults to ViewVersion fields#3458
ebyhr wants to merge 1 commit into
apache:mainfrom
ebyhr:ebi/view-default

Conversation

@ebyhr
Copy link
Copy Markdown
Member

@ebyhr ebyhr commented Jun 5, 2026

Rationale for this change

Allow users to create a view with fewer parameters.

We can set an environment context (engine-name and engine-version) in summary field by default
once #3441 is merged.

Are these changes tested?

Yes

Are there any user-facing changes?

No

version_id=1,
timestamp_ms=12345,
schema_id=1,
summary={"engine-name": "spark", "engineVersion": "3.3"},
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.

We should keep this summary here, since it's not the default.

version_id=1,
timestamp_ms=12345,
schema_id=1,
summary={"engine-name": "spark", "engineVersion": "3.3"},
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.

Keep this summary since not default.

timestamp_ms: int = Field(alias="timestamp-ms", default=int(time.time() * 1000))
"""Timestamp when the version was created (ms from epoch)"""
summary: dict[str, str] = Field()
summary: dict[str, str] = Field(default={})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we need to use default_factory instead:

Suggested change
summary: dict[str, str] = Field(default={})
summary: dict[str, str] = Field(default_factory=dict)

schema_id: int = Field(alias="schema-id")
"""ID of the schema for the view version"""
timestamp_ms: int = Field(alias="timestamp-ms")
timestamp_ms: int = Field(alias="timestamp-ms", default=int(time.time() * 1000))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we want to do:

Suggested change
timestamp_ms: int = Field(alias="timestamp-ms", default=int(time.time() * 1000))
timestamp_ms: int = Field(alias="timestamp-ms", default_factory=lambda: int(time.time() * 1000))

Otherwise the value will be static.

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.

3 participants