Skip to content

fix(types-timestamptz): Handle MarshalText and String for pointer receivers#596

Closed
erezrokah wants to merge 1 commit intocloudquery:mainfrom
erezrokah:fix/support_timestamp_pointers
Closed

fix(types-timestamptz): Handle MarshalText and String for pointer receivers#596
erezrokah wants to merge 1 commit intocloudquery:mainfrom
erezrokah:fix/support_timestamp_pointers

Conversation

@erezrokah
Copy link
Copy Markdown
Member

@erezrokah erezrokah commented Jan 10, 2023

Summary

We need to also try calling MarshalText and String when those methods are declared on a pointer receiver


Use the following steps to ensure your PR is ready to be reviewed

  • Read the contribution guidelines 🧑‍🎓
  • Run go fmt to format your code 🖊
  • Lint your changes via golangci-lint run 🚨 (install golangci-lint here)
  • Update or add tests 🧪
  • Ensure the status checks below are successful ✅

@erezrokah erezrokah changed the title fix(types-timestamptz): Handle MarshalText and String support on pointer receivers fix(types-timestamptz): Handle MarshalText and String for pointer receivers Jan 10, 2023
@github-actions github-actions bot added the fix label Jan 10, 2023
@github-actions github-actions bot added fix and removed fix labels Jan 10, 2023
@github-actions
Copy link
Copy Markdown

⏱️ Benchmark results

Comparing with 506d4cc

  • DefaultConcurrencyDFS-2 resources/s: 11,205 ⬆️ 1.99% increase vs. 506d4cc
  • DefaultConcurrencyRoundRobin-2 resources/s: 10,885 ⬇️ 0.34% decrease vs. 506d4cc
  • Glob-2 ns/op: 218.4 ⬆️ 12.87% increase vs. 506d4cc
  • TablesWithChildrenDFS-2 resources/s: 25,198 ⬆️ 0.36% increase vs. 506d4cc
  • TablesWithChildrenRoundRobin-2 resources/s: 23,868 ⬇️ 13.41% decrease vs. 506d4cc
  • TablesWithRateLimitingDFS-2 resources/s: 28.45 ⬆️ 0.42% increase vs. 506d4cc
  • TablesWithRateLimitingRoundRobin-2 resources/s: 801.2 ⬇️ 2.78% decrease vs. 506d4cc
  • BufferedScanner-2 ns/op: 12.98 ⬆️ 6.63% increase vs. 506d4cc
  • LogReader-2 ns/op: 39.91 ⬆️ 13.33% increase vs. 506d4cc

@yevgenypats
Copy link
Copy Markdown
Contributor

@erezrokah can you link to where this is used/current failing in the plugins ?

@erezrokah
Copy link
Copy Markdown
Member Author

I thought I needed it for GCP, but actually the fix there was to override the resolver. I still think it makes sense as it's common to declare methods on pointer receivers.

I don't think it's super important at the moment so we can close this

@erezrokah erezrokah closed this Jan 11, 2023
@erezrokah erezrokah deleted the fix/support_timestamp_pointers branch January 11, 2023 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants