Skip to content

feat: Add provisions for storing and inheriting time zone information in relational objects #1102

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

krlmlr
Copy link
Collaborator

@krlmlr krlmlr commented Apr 18, 2025

No description provided.

@krlmlr krlmlr enabled auto-merge (squash) April 18, 2025 12:02
@krlmlr krlmlr marked this pull request as draft April 18, 2025 12:03
auto-merge was automatically disabled April 18, 2025 12:03

Pull request was converted to draft

@krlmlr krlmlr requested a review from Copilot April 18, 2025 12:03
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a new feature to support storing and inheriting time zone information in relational objects. Key changes include:

  • Updating external relation creation functions in both src/reltoaltrep.cpp and src/relational.cpp to pass an additional time zone parameter.
  • Modifying existing API calls to include parent relation pointers and a new default time zone value.
  • Enhancing the RelationWrapper in src/include/rapi.hpp with new constructors and a tzone member for improved time zone propagation.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/reltoaltrep.cpp Updated external relation creation to include a parent relation pointer for time zone inheritance.
src/relational.cpp Added a default tzone parameter and updated various relation functions to propagate time zone details.
src/include/rapi.hpp Extended RelationWrapper with overloads that accept explicit and inherited time zone values.
Comments suppressed due to low confidence (3)

src/relational.cpp:155

  • [nitpick] Consider adding test cases to verify that the default time zone value is correctly propagated across the updated relation functions.
std::string tzone = "";

src/include/rapi.hpp:137

  • [nitpick] Consider renaming 'rel_orig_1' and 'rel_orig_2' to more descriptive names (e.g., 'primary_relation' and 'secondary_relation') to clarify their roles in time zone inheritance.
RelationWrapper(duckdb::shared_ptr<Relation> rel_p, const cpp11::external_pointer<RelationWrapper>& rel_orig_1, const cpp11::external_pointer<RelationWrapper>& rel_orig_2) : rel(std::move(rel_p)) {

src/reltoaltrep.cpp:513

  • [nitpick] If 'wrapper->rel_eptr' represents an original relation wrapper for time zone propagation, consider renaming it to better reflect its role.
return make_external<RelationWrapper>("duckdb_relation", rel, wrapper->rel_eptr);

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.

1 participant