Repository navigation
[ruby-compat/08] Match ruby's timestamp-to-INTEGER/FLOAT/STRING output - #117
Conversation
INTEGER/FLOAT now output epoch seconds instead of milliseconds, and default_timestamp_format no longer appends a timezone offset, matching ruby's timestamp_converter and DEFAULT_TIMESTAMP_FORMAT. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…mp-conversion-match-ruby Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
ありがとうございます。変換ロジックの本体は ruby の value_converter_factory.rb(val.to_i / val.to_f / DEFAULT_TIMESTAMP_FORMAT = "%Y-%m-%d %H:%M:%S.%6N")と正確に一致しています。1970 年以前の INTEGER の floor 挙動や、現実的な値での FLOAT の精度も ruby と揃っていることを確認しました。破壊的変更が PR 本文と README に明記されている点も良いと思います。
マージ前に対応をお願いしたい点が 2 つあります。
-
timestamp → STRING/DATE/DATETIME で
default_timezoneが依然として無視されており、デフォルトフォーマットから%:zを外したことでそれが見えなくなっています。BigqueryColumnOption.getTimezone()は"UTC"固定のデフォルトで、PluginTask.getDefaultTimezone()はsrc/mainのどこからも参照されていません。ruby は@timezone = timezone || default_timezoneとしています。default_timezone: Asia/Tokyoとcolumn_options: [{name: ts, type: STRING}]の組み合わせで、ruby は2020-05-01 09:00:00.000000を出力しますが、このブランチは2020-05-01 00:00:00.000000を出力し、オフセットもないためズレに気づけません。既存の問題ではありますが、この PR のタイトルは STRING 出力が ruby と一致すると謳っています。この PR でフォールバックを実装する(column option をOptional<String>+@ConfigDefault("null")にしてorElse(task.getDefaultTimezone())する)か、PR 本文と #122 に「後回しにする」と明記するか、どちらかをお願いします。 -
テストが新しい演算の挙動を固定できていません。 インラインコメントを参照してください。INTEGER のケースは秒単位ぴったりの値を使っているため、切り捨てか四捨五入か、1970 年以前の floor 挙動かが検証されておらず、FLOAT の許容誤差も必要以上に緩くなっています。
ブロッカーではない nit: INTEGER/FLOAT 列でミリ秒に依存していたユーザー向けの移行案内がありません。STRING のユーザーは default_timestamp_format: "%Y-%m-%d %H:%M:%S.%6N %:z" で元に戻せるので、README に一行あると親切です。INTEGER/FLOAT でミリ秒が必要なユーザーには戻す手段がなく、失敗も静かに起きます(下流の TIMESTAMP_MILLIS(col) が 1970 年の日付を返すなど)。
| node.put(name, src.getEpochSecond() + src.getNano() / 1_000_000_000.0); | ||
| break; | ||
| case STRING: | ||
| String format = columnOption.getTimestampFormat().orElse(task.getDefaultTimestampFormat()); |
There was a problem hiding this comment.
数行下で使われているタイムゾーン(timezone = columnOption.getTimezone())は UTC 固定のデフォルトで、ruby の timezone || default_timezone と違い task.getDefaultTimezone() へのフォールバックがありません。デフォルトフォーマットに %:z がなくなったため、default_timezone: Asia/Tokyo を設定しているユーザーは、ruby の出力から 9 時間ズレたタイムゾーンなしの文字列を静かに受け取ることになります。下の DATE / DATETIME 分岐も同様です。ここでフォールバックを実装するか、PR 本文で「後回し」と明記するかをお願いします。
| break; | ||
| case FLOAT: | ||
| node.put(name, Double.valueOf(src.toEpochMilli())); | ||
| node.put(name, src.getEpochSecond() + src.getNano() / 1_000_000_000.0); |
There was a problem hiding this comment.
Nit: この加算は epoch 直近では正しく丸められません。Instant.ofEpochMilli(-1) だと -1 + 0.999 = -0.0010000000000000009 になり、ruby の to_f は -0.001 を返します。実際の日付では差は最大 1 ulp なのでこのままでも問題ありません。完全に一致させたい場合は BigDecimal.valueOf(src.getEpochSecond()).add(BigDecimal.valueOf(src.getNano(), 9)).doubleValue() で実現できます。
| BigqueryTimestampConverter.convertAndSet( | ||
| node, "key", ts, BigqueryColumnOptionType.INTEGER, columnOption, task); | ||
| assertEquals(1588291200000L, node.get("key").asLong()); | ||
| assertEquals(1588291200L, node.get("key").asLong()); |
There was a problem hiding this comment.
この値は秒単位ぴったりなので、getEpochSecond() でも toEpochMilli() / 1000 でも Math.round(toEpochMilli() / 1000.0) でも通ってしまいます。ruby の to_i と同じ挙動を固定するケースを追加してください。
ofEpochMilli(1588291200999L)→1588291200L(四捨五入ではなく切り捨て)ofEpochMilli(-1L)→-1L(ゼロ方向の切り捨てではなく floor)
| org.embulk.spi.time.Timestamp ts = org.embulk.spi.time.Timestamp.ofEpochMilli(1588291200123L); | ||
|
|
||
| BigqueryTimestampConverter.convertAndSet( | ||
| node, "key", ts, BigqueryColumnOptionType.FLOAT, columnOption, task); |
There was a problem hiding this comment.
1588291200L + 123000000 / 1e9 は double として正確に 1588291200.123 になるので、delta は 0.0(または 1e-9)にできます。1970 年以前のケース(ofEpochMilli(-500L) → -0.5)もここに追加しておくと良いと思います。
…havior Cover the review feedback on the timestamp conversion change: - INTEGER: sub-second values are truncated (999ms -> same second) and pre-1970 values are floored (-1ms -> -1), matching ruby's Time#to_i. - FLOAT: the fraction case now asserts an exact double, plus pre-1970 cases (-500ms -> -0.5, -1ms -> -0.001 as ruby's Time#to_f rounds once). - default_timezone: timestamp -> STRING/DATE/DATETIME, string -> TIMESTAMP, and nested RECORD TIMESTAMP fields fall back to task default_timezone when the column/field has no timezone, as ruby's `timezone || default_timezone` does. The -1ms FLOAT case and the default_timezone cases fail until the converter is updated; the rest already pass and only guard the behavior. TestBigqueryRecordConverter drops its @before fixture for a createTask() helper so a test can build a task with a different default_timezone. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
column_options[].timezone and fields[].timezone defaulted to "UTC" on the option itself, so task-level default_timezone was never read and timestamp -> STRING/DATE/DATETIME (and string/nested-field parsing) ignored it. Make timezone Optional and resolve `timezone || default_timezone` at every use, as ruby's ValueConverterFactory does. FLOAT conversion added epoch seconds and a nanosecond fraction as doubles, which rounds twice and differs from ruby's Time#to_f near the epoch (-1ms gave -0.0010000000000000009). Sum via BigDecimal and round once. README gains an upgrade note: how to restore the old offset-bearing STRING default, and that INTEGER/FLOAT milliseconds can't be restored. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The base INTEGER test now uses a sub-second value so it pins truncation on its own, replacing the separate truncation test; the pre-1970 floor test moves next to it. For FLOAT, the -1ms case already covers a negative value, so it takes over the beforeEpoch name and the redundant -500ms case is dropped. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@john-abre
|
…mp-conversion-match-ruby # Conflicts: # README.md
john-abre
left a comment
There was a problem hiding this comment.
@chikamura ありがとうございます!
Blockerはないですが、#124 のマージでの微調整でどう?
(メモ to self: timestamp_formatありtimezone無いカラムはこの修正でdefault_timezoneになりますので、今までJava版プラグインを使っていた顧客の状況は要確認 👀 )
|
@john-abre That said, while we're working through the ruby-compatibility changes, I'd like to keep the scope of |
Summary
timestamp_converteroutputs epoch seconds for INTEGER/FLOAT (val.to_i/val.to_f), and itsDEFAULT_TIMESTAMP_FORMAThas no timezone offset. The java port previously output epoch milliseconds for INTEGER/FLOAT and appended a timezone offset (%:z) to the default STRING format.getEpochSecond()and FLOAT to outputgetEpochSecond() + getNano() / 1e9, matching ruby'sto_i/to_f.default_timestamp_format's@ConfigDefaultfrom"%Y-%m-%d %H:%M:%S.%6N %:z"to"%Y-%m-%d %H:%M:%S.%6N", matching ruby'sDEFAULT_TIMESTAMP_FORMAT(this also brings the code in line with README, which already documented the no-offset format).timestampcolumns asINTEGER/FLOAT, or asSTRINGwithout an explicittimestamp_format, will see different output values/format.Test plan
./gradlew test(addedtestConvertTimestampToFloat_withFractionto cover the new sub-second FLOAT arithmetic)./gradlew spotlessApply🤖 Generated with Claude Code