Skip to content
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

add TIME type conversion to string converter #168

Merged
merged 6 commits into from
Aug 27, 2024

Conversation

p-eye
Copy link
Contributor

@p-eye p-eye commented Jul 22, 2024

The converter for converting STRING to DATE type is working well,
but while converting a STRING to TIME type, an error occurred because a converter for this type does not exist. The error message is (NotSupportedType) cannot take column type TIME for string column.

To fix this, I have added a converter to transform STRING type into BigQuery TIME type. I only updated the string_converter since the format "%H:%M:%S.%6N" is not compatible with the TIMESTAMP type, so I did not modify the timestamp_converter.

--- my bigquery table ---
image

--- works well with DATE column ---

in:
  type: inline
  schema:
    - {name: date_column, type: string}
  data:
    - {date_column: "2024-01-01"}
out:
  type: bigquery
  mode: append
  auth_method: service_account
  json_keyfile: my_keyfile
  location: asia-northeast3
  project: my_project
  destination_project: my-destination-project
  compression: GZIP
  source_format: NEWLINE_DELIMITED_JSON
  default_timezone: Asia/Seoul
  dataset: my_dataset
  table: my_table
  column_options:
  - name: date_column
    type: DATE

--- not works with TIME column ---

in:
  type: inline
  schema:
    - {name: time_column, type: string}
  data:
    - {time_column: "00:03:22"}
out:
  type: bigquery
  mode: append
  auth_method: service_account
  json_keyfile: my_keyfile
  location: asia-northeast3
  project: my-project
  destination_project: my-destination-project
  compression: GZIP
  source_format: NEWLINE_DELIMITED_JSON
  default_timezone: Asia/Seoul
  dataset: my_dataset
  table: my_table
  column_options:
  - name: time_column
    type: TIME

@p-eye
Copy link
Contributor Author

p-eye commented Jul 23, 2024

@hiroyuki-sato
Hi, I opened a new PR. Could you please review it?
thanks

Copy link
Member

@hiroyuki-sato hiroyuki-sato left a comment

Choose a reason for hiding this comment

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

@p-eye Could you check my comments? (Thank you for proposing PR.)

@p-eye p-eye force-pushed the fix_string_to_time_converter branch from 28fdc3a to 563872b Compare July 24, 2024 23:45
@p-eye p-eye force-pushed the fix_string_to_time_converter branch from 563872b to 8eaab04 Compare July 24, 2024 23:48
@dp-daesung
Copy link

This is exactly what I need. I'm waiting for it to merge.

Copy link
Member

@hiroyuki-sato hiroyuki-sato left a comment

Choose a reason for hiding this comment

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

Thanks. I'll approve after removing trailing spaces.

Copy link
Member

@hiroyuki-sato hiroyuki-sato left a comment

Choose a reason for hiding this comment

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

@p-eye LGTM 👍 Thanks! @joker1007 Could you take a look when you get a chance?

@p-eye
Copy link
Contributor Author

p-eye commented Aug 26, 2024

hi, any updates? 😃

@hiroyuki-sato
Copy link
Member

@p-eye I pinged reviewer. The maintainer will review it soon. Please wait a bit

Copy link
Contributor

@joker1007 joker1007 left a comment

Choose a reason for hiding this comment

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

I understand. LGTM.

@hiroyuki-sato hiroyuki-sato merged commit e9b1c03 into embulk:master Aug 27, 2024
2 checks passed
@hiroyuki-sato
Copy link
Member

Thanks!

@hiroyuki-sato
Copy link
Member

@p-eye , @dp-daesung embulk-output-bigquery 0.7.3 has been released. Enjoy.
@p-eye Thank you for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants