Skip to content

Add custom classes to represent Google Well Known Types#1271

Merged
hectorcast-db merged 9 commits intomainfrom
hectorcast-db/well-known-type-types
Sep 3, 2025
Merged

Add custom classes to represent Google Well Known Types#1271
hectorcast-db merged 9 commits intomainfrom
hectorcast-db/well-known-type-types

Conversation

@hectorcast-db
Copy link
Copy Markdown
Contributor

@hectorcast-db hectorcast-db commented Aug 19, 2025

What changes are proposed in this pull request?

Add custom classes to represent Google Well Known Types

These classes have custom serialization:
https://protobuf.dev/reference/protobuf/google.protobuf/

How is this tested?

Added unit tests.

NO_CHANGELOG=true

Comment thread common/types/duration/duration.go Outdated
Comment thread common/types/duration/duration.go Outdated
Comment thread common/types/duration/duration.go Outdated
Comment thread common/types/fieldmask/fieldmask_test.go Outdated
Comment thread common/types/time/time.go Outdated
Comment thread common/types/time/time.go Outdated
Comment thread common/types/time/time_test.go
Copy link
Copy Markdown
Contributor

@parthban-db parthban-db left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment thread common/types/duration/duration.go Outdated
Comment thread common/types/duration/duration.go Outdated
Comment thread common/types/duration/duration.go Outdated
Comment thread common/types/duration/duration.go Outdated
Copy link
Copy Markdown
Contributor

@renaudhartert-db renaudhartert-db left a comment

Choose a reason for hiding this comment

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

Should we use the Proto classes for internal (de)serialization with the protojson package?

Comment thread common/types/time/time.go Outdated
Comment thread common/types/time/time.go
Comment thread common/types/time/time.go Outdated
@hectorcast-db
Copy link
Copy Markdown
Contributor Author

Should we use the Proto classes for internal (de)serialization with the protojson package?

We specifically cannot use Proto FieldMask because it encapsulates the descriptor of the message, and validates the content.

var messageType *descriptorpb.DescriptorProto
fm, err := fieldmaskpb.New(messageType, "field.name", "field.number")

Except for that one, we could use the others. But this way we are slightly more consistent.
We also still have the original implementation of custom types for Python which copies the native types, and so we would need to change that again.

@parthban-db
Copy link
Copy Markdown
Contributor

We specifically cannot use Proto FieldMask because it encapsulates the descriptor of the message, and validates the content.

I think we can use protoLibrary for marshalling/unmarshalling the fieldMask. However, we can't use the standard function to create it, as it needs a proto message. Instead, we can manually set the Paths field in the fieldMask message.

@hectorcast-db
Copy link
Copy Markdown
Contributor Author

I think we can use protoLibrary for marshalling/unmarshalling the fieldMask. However, we can't use the standard function to create it, as it needs a proto message. Instead, we can manually set the Paths field in the fieldMask message.

Mmm... We can do that with the others. Like,

type Time struct {
 time.Time
}

func (t Time) Marshall() ([]byte, error) {
  pbTime := timepb.New(t.Time)
  return proto.Marshall(pbTime)
}

Thoughts?

@parthban-db
Copy link
Copy Markdown
Contributor

Mmm... We can do that with the others. Like,
type Time struct {
time.Time
}
func (t Time) Marshall() ([]byte, error) {
pbTime := timepb.New(t.Time)
return proto.Marshall(pbTime)
}
Thoughts?

Looks good, but why are we storing this as time.Time and not timepb.Time?

@hectorcast-db
Copy link
Copy Markdown
Contributor Author

hectorcast-db commented Aug 28, 2025

Looks good, but why are we storing this as time.Time and not timepb.Time?

We are exposing this to the customer. I guess is a matter of preference on what we want the user to see:

type Time struct {
  time.Time //exposes go time 
}

type Time struct {
  timepb.Time // exposes pb time
}

type Time struct {
  internal <either> //exposes nothing
}

In python we did our own classes which "mimic" the python native time. So exposing native time, while not the same, seems slightly more consistent and requires less rework. Right now:

class Timestamp:
   def __init__(self, seconds: int = 0, nanos: int = 0) -> None:
     self.seconds = seconds
     self.nanos = nanos

We can change it again, but python this was already done, reviews, tested and ready to go.

@hectorcast-db hectorcast-db force-pushed the hectorcast-db/well-known-type-types branch from 7e6b79e to 6cd5094 Compare September 2, 2025 13:22
@hectorcast-db hectorcast-db requested a review from rauchy September 3, 2025 09:42
Comment thread common/types/fieldmask/fieldmask.go Outdated
// field mask from a string according to Google Well Known Type
func (f *FieldMask) UnmarshalJSON(data []byte) error {
if f == nil {
return fmt.Errorf("json.Unmarshal on nil pointer")
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.

Suggested change
return fmt.Errorf("json.Unmarshal on nil pointer")
return fmt.Errorf("FieldMask.UnmarshalJSON on nil pointer")

Comment thread common/types/duration/duration.go Outdated
//
// Example:
//
// customDur := durationpb.New(30 * time.Second)
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.

Suggested change
// customDur := durationpb.New(30 * time.Second)
// customDur := duration.New(30 * time.Second)

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Sep 3, 2025

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/sdk-go

Inputs:

  • PR number: 1271
  • Commit SHA: 38b4cb979094db4eca55e60c8ffb07a8a4da6112

Checks will be approved automatically on success.

@hectorcast-db hectorcast-db dismissed renaudhartert-db’s stale review September 3, 2025 15:32

Discussed with the team and already applied the relevant changes.

@hectorcast-db hectorcast-db added this pull request to the merge queue Sep 3, 2025
Merged via the queue into main with commit 19c3640 Sep 3, 2025
15 checks passed
@hectorcast-db hectorcast-db deleted the hectorcast-db/well-known-type-types branch September 3, 2025 15:34
github-merge-queue Bot pushed a commit to databricks/cli that referenced this pull request Feb 2, 2026
## Changes

Implements conversion support for SDK native types across the dyn
package conversion layer (FromTyped, ToTyped, Normalize). Supports three
SDK types that use custom JSON marshaling:

- duration.Duration: Protobuf duration format (e.g., "300s")
- time.Time: RFC3339 timestamp format (e.g., "2023-12-25T10:30:00Z")
- fieldmask.FieldMask: Comma-separated paths (e.g., "name,age,email")

The implementation uses a generic approach with all SDK-specific code in
sdk_native_types.go and sdk_native_types_test.go.

## Why

The new Postgres APIs use these types and need the to/from conversion to
work.

Also see databricks/databricks-sdk-go#1271.

---------

Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
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.

4 participants