You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "suneet-s (via GitHub)" <gi...@apache.org> on 2025/03/03 21:29:19 UTC
[PR] API: Speed up Timestamps#toHumanString [iceberg]
suneet-s opened a new pull request, #12447:
URL: https://github.com/apache/iceberg/pull/12447
String#format uses regexes internally which can be expensive when used repeatedly. This PR switches out the implementation for a char array implementation that pads zeros as needed.
A simple benchmark was written to compare these 2 implementations which shows an improvement from ~ 300ns per op to 15ns per op
```
Benchmark (monthOrdinal) Mode Cnt Score Error Units
DateFormattingBenchmark.charArrayFormat 0 avgt 10 17.580 ± 5.029 ns/op
DateFormattingBenchmark.charArrayFormat 1 avgt 10 16.082 ± 0.220 ns/op
DateFormattingBenchmark.charArrayFormat 12 avgt 10 13.504 ± 0.151 ns/op
DateFormattingBenchmark.charArrayFormat 15 avgt 10 13.541 ± 0.684 ns/op
DateFormattingBenchmark.charArrayFormat 123 avgt 10 17.616 ± 5.112 ns/op
DateFormattingBenchmark.charArrayFormat 1234 avgt 10 13.350 ± 0.544 ns/op
DateFormattingBenchmark.charArrayFormat 12345 avgt 10 13.656 ± 0.128 ns/op
DateFormattingBenchmark.stringFormat 0 avgt 10 296.576 ± 1.027 ns/op
DateFormattingBenchmark.stringFormat 1 avgt 10 356.724 ± 277.416 ns/op
DateFormattingBenchmark.stringFormat 12 avgt 10 320.441 ± 1.802 ns/op
DateFormattingBenchmark.stringFormat 15 avgt 10 298.042 ± 11.098 ns/op
DateFormattingBenchmark.stringFormat 123 avgt 10 302.227 ± 11.005 ns/op
DateFormattingBenchmark.stringFormat 1234 avgt 10 309.586 ± 11.386 ns/op
DateFormattingBenchmark.stringFormat 12345 avgt 10 305.583 ± 1.682 ns/op
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Speed up Timestamps#toHumanString [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12447:
URL: https://github.com/apache/iceberg/pull/12447#discussion_r1989658472
##########
api/src/test/java/org/apache/iceberg/transforms/TestTransformUtil.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.transforms;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.stream.Stream;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.CsvSource;
+
+public class TestTransformUtil {
+
+ @ParameterizedTest
+ @CsvSource({"0, 1970", "1, 1971", "10, 1980", "47, 2017", "100, 2070"})
Review Comment:
We should probably have tests for 3 digit years. I'm not sure we have the same behavior as we used to there
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Speed up Timestamps#toHumanString [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12447:
URL: https://github.com/apache/iceberg/pull/12447#issuecomment-2816385436
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Speed up Timestamps#toHumanString [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #12447:
URL: https://github.com/apache/iceberg/pull/12447#issuecomment-2715305062
Can you see who the caller is there? I'm just interested in why flink is calling that function so often
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Speed up Timestamps#toHumanString [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12447:
URL: https://github.com/apache/iceberg/pull/12447#discussion_r1989662498
##########
api/src/test/java/org/apache/iceberg/transforms/TestTransformUtil.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.transforms;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.stream.Stream;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.CsvSource;
+
+public class TestTransformUtil {
+
+ @ParameterizedTest
+ @CsvSource({"0, 1970", "1, 1971", "10, 1980", "47, 2017", "100, 2070"})
Review Comment:
Rather than testing a few isolated values, it's probably better to randomly generate a few ten's of thousands of random dates and make sure they process as expected. Just fix the seed in the test suite for repeatability
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Speed up Timestamps#toHumanString [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12447:
URL: https://github.com/apache/iceberg/pull/12447#issuecomment-2795474384
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Speed up Timestamps#toHumanString [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #12447: API: Speed up Timestamps#toHumanString
URL: https://github.com/apache/iceberg/pull/12447
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Speed up Timestamps#toHumanString [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12447:
URL: https://github.com/apache/iceberg/pull/12447#discussion_r1989657396
##########
api/src/test/java/org/apache/iceberg/transforms/TestTransformUtil.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.transforms;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.stream.Stream;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.CsvSource;
+
+public class TestTransformUtil {
+
+ @ParameterizedTest
+ @CsvSource({"0, 1970", "1, 1971", "10, 1980", "47, 2017", "100, 2070"})
+ public void test_humanYear(int yearOrdinal, String expected) {
+ String result = TransformUtil.humanYear(yearOrdinal);
+ assertThat(result).isEqualTo(expected);
+ }
+
+ private static Stream<Arguments> humanYears() {
Review Comment:
Is this used?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Speed up Timestamps#toHumanString [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #12447:
URL: https://github.com/apache/iceberg/pull/12447#issuecomment-2714915616
@suneet-s What is the motivation for this change? Is toHumanString on a critical path somewhere? I don't think I have an issue with putting in a faster implementation as long as it is easy to check for correctness.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Speed up Timestamps#toHumanString [iceberg]
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s commented on PR #12447:
URL: https://github.com/apache/iceberg/pull/12447#issuecomment-2715260800
@RussellSpitzer Thanks for reviewing this change. In a performance test we were running where a flink pipieline was writing data to an iceberg table, we saw that this function was taking ~ 9% of the time in the flamegraphs we collected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org