Add Pacific/Kanton, Europe/Kyiv, America/Ciudad_Juarez time zones

[wypb]

Presto 0.282 adds three new timezones: Pacific/Kanton, Europe/Kyiv, America/Ciudad_Juarez: prestodb/presto#19758, So velox should be consistent with it

[netlify]

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	313f145
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/650c12194b0ddc000858e8c1

[mbasmanova]

@wypb Thanks. Did you re-generate the TimeZoneDatabase.cpp file or edited manually?

[facebook-github-bot]

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[mbasmanova]

@wypb In case you edited the file manually, please, re-generate the file and update the PR.

// This file is generated. Do not modify it manually. To re-generate it, run:
//
//  ./velox/type/tz/gen_timezone_database.py -f /tmp/zone-index.properties \
//       > velox/type/tz/TimeZoneDatabase.cpp
//
// The zone-index.properties file should be the same one used in Presto,
// Available here :
// https://github.com/prestodb/presto/blob/master/presto-common/src/main/resources/com/facebook/presto/common/type/zone-index.properties.
// @generated

[wypb]

@mbasmanova I generate the file using velox/type/tz/gen_timezone_database.py script, but the code format verification would fail, so I manually modified some code:

➜  velox git:(add_timezone) ✗ scripts/check.py format main
Fix  : velox/type/tz/TimeZoneDatabase.cpp
diff --git a/velox/type/tz/TimeZoneDatabase.cpp b/velox/type/tz/TimeZoneDatabase.cpp
index ff69262fd..9687e4d20 100644
--- a/velox/type/tz/TimeZoneDatabase.cpp
+++ b/velox/type/tz/TimeZoneDatabase.cpp
@@ -20,11 +20,12 @@
 //       > velox/type/tz/TimeZoneDatabase.cpp
 //
 // The zone-index.properties file should be the same one used in Presto,
-// Available here : https://github.com/prestodb/presto/blob/master/presto-common/src/main/resources/com/facebook/presto/common/type/zone-index.properties.
+// Available here :
+// https://github.com/prestodb/presto/blob/master/presto-common/src/main/resources/com/facebook/presto/common/type/zone-index.properties.
 // @generated

-#include <unordered_map>
 #include <string>
+#include <unordered_map>

 namespace facebook::velox::util {
Ok   : velox/type/tz/tests/TimeZoneMapTest.cpp

If necessary, I can help modify the ./velox/type/tz/gen_timezone_database.py script to make the generated code pass code format verification

[mbasmanova]

@wypb Thanks. It might be helpful to add make target to top-level Makefile to re-generate this file + run make format-fix afterwards. Maybe, create a GitHub issue.

CC: @majetideepak @kgpai @assignUser

[wypb]

Hi @mbasmanova I modified part of the logic of velox/type/tz/gen_timezone_database.py and now the generated code can be passed through scripts/check.py format main. Could you help me review this again, thanks.

[facebook-github-bot]

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[mbasmanova]

@wypb Thanks.

[wypb]

Hi @mbasmanova, AggregationFuzzer failed to run. It seems to have nothing to do with this PR. Could you please help to confirm it?

I20230921 10:51:06.731838 197931 AggregationFuzzer.cpp:567] Persisted aggregation plans to /tmp/spark_aggregate_fuzzer_repro/velox_aggregationVerifier_Ftwu9T/plan_nodes
terminate called after throwing an instance of 'facebook::velox::VeloxRuntimeError'
  what():  Exception: VeloxRuntimeError
Error Source: RUNTIME
Error Code: INVALID_STATE
Reason: (-1583242847 vs. 0) Index must not be negative
Retriable: False
Expression: idx >= 0
Function: isNullAt
File: ../.././velox/vector/BaseVector.h
Line: 155
Stack trace:
# 0  _ZN8facebook5velox7process10StackTraceC1Ei
# 1  _ZN8facebook5velox14VeloxException5State4makeIZNS1_C4EPKcmS5_St17basic_string_viewIcSt11char_traitsIcEES9_S9_S9_bNS1_4TypeES9_EUlRT_E_EESt10shared_ptrIKS2_ESA_SB_
# 2  _ZN8facebook5velox14VeloxExceptionC1EPKcmS3_St17basic_string_viewIcSt11char_traitsIcEES7_S7_S7_bNS1_4TypeES7_
# 3  _ZN8facebook5velox17VeloxRuntimeErrorC2EPKcmS3_St17basic_string_viewIcSt11char_traitsIcEES7_S7_S7_bS7_
# 4  _ZN8facebook5velox6detail14veloxCheckFailINS0_17VeloxRuntimeErrorERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEEEvRKNS1_18VeloxCheckFailArgsET0_
# 5  _ZNK8facebook5velox10BaseVector8isNullAtEi
# 6  _ZN8facebook5velox4exec12_GLOBAL__N_113compareArraysERNS0_10ByteStreamERKNS0_10BaseVectorEiiNS0_12CompareFlagsE
# 7  _ZN8facebook5velox4exec12_GLOBAL__N_17compareILNS0_8TypeKindE30EEESt8optionalIiERNS0_10ByteStreamERKNS0_10BaseVectorEiNS0_12CompareFlagsE
# 8  _ZZN8facebook5velox4exec12_GLOBAL__N_113compareSwitchERNS0_10ByteStreamERKNS0_10BaseVectorEiNS0_12CompareFlagsEENKUlvE_clEv
# 9  _ZN8facebook5velox4exec12_GLOBAL__N_113compareSwitchERNS0_10ByteStreamERKNS0_10BaseVectorEiNS0_12CompareFlagsE
# 10 _ZN8facebook5velox4exec17ContainerRowSerde16compareWithNullsERNS0_10ByteStreamERKNS0_13DecodedVectorEiNS0_12CompareFlagsE
# 11 _ZNK8facebook5velox9functions9aggregate22SingleValueAccumulator7compareERKNS0_13DecodedVectorEiNS0_12CompareFlagsE
# 12 _ZN8facebook5velox9functions9aggregate8sparksql12_GLOBAL__N_115SparkComparatorILb0ENS0_11ComplexTypeENS2_22SingleValueAccumulatorEE7compareEPS7_RKNS0_13DecodedVectorEib
# 13 _ZZN8facebook5velox9functions9aggregate21MinMaxByAggregateBaseINS0_11ComplexTypeES4_Lb0ENS2_8sparksql12_GLOBAL__N_115SparkComparatorEE11addRawInputEPPcRKNS0_17SelectivityVectorERKSt6vectorISt10shared_ptrINS0_10BaseVectorEESaISH_EEbENKUlPT_RKT0_T1_T2_E_clINS2_22SingleValueAccumulatorENS0_13DecodedVectorEibEEDaSN_SQ_SR_SS_
# 14 _ZN8facebook5velox9functions9aggregate21MinMaxByAggregateBaseINS0_11ComplexTypeES4_Lb0ENS2_8sparksql12_GLOBAL__N_115SparkComparatorEE12updateValuesIZNS8_11addRawInputEPPcRKNS0_17SelectivityVectorERKSt6vectorISt10shared_ptrINS0_10BaseVectorEESaISI_EEbEUlPT_RKT0_T1_T2_E_EEvSA_RKNS0_13DecodedVectorESX_ibSN_
# 15 _ZZN8facebook5velox9functions9aggregate21MinMaxByAggregateBaseINS0_11ComplexTypeES4_Lb0ENS2_8sparksql12_GLOBAL__N_115SparkComparatorEE11addRawInputIZNS8_11addRawInputEPPcRKNS0_17SelectivityVectorERKSt6vectorISt10shared_ptrINS0_10BaseVectorEESaISI_EEbEUlPT_RKT0_T1_T2_E_EEvSB_SE_SM_SN_ENKUliE_clEi
# 16 _ZNK8facebook5velox17SelectivityVector15applyToSelectedIZNS0_9functions9aggregate21MinMaxByAggregateBaseINS0_11ComplexTypeES6_Lb0ENS4_8sparksql12_GLOBAL__N_115SparkComparatorEE11addRawInputIZNSA_11addRawInputEPPcRKS1_RKSt6vectorISt10shared_ptrINS0_10BaseVectorEESaISJ_EEbEUlPT_RKT0_T1_T2_E_EEvSD_SF_SN_SO_EUliE_EEvSO_
# 17 _ZN8facebook5velox9functions9aggregate21MinMaxByAggregateBaseINS0_11ComplexTypeES4_Lb0ENS2_8sparksql12_GLOBAL__N_115SparkComparatorEE11addRawInputIZNS8_11addRawInputEPPcRKNS0_17SelectivityVectorERKSt6vectorISt10shared_ptrINS0_10BaseVectorEESaISI_EEbEUlPT_RKT0_T1_T2_E_EEvSB_SE_SM_SN_
# 18 _ZN8facebook5velox9functions9aggregate21MinMaxByAggregateBaseINS0_11ComplexTypeES4_Lb0ENS2_8sparksql12_GLOBAL__N_115SparkComparatorEE11addRawInputEPPcRKNS0_17SelectivityVectorERKSt6vectorISt10shared_ptrINS0_10BaseVectorEESaISH_EEb
# 19 _ZN8facebook5velox4exec11GroupingSet21addInputForActiveRowsERKSt10shared_ptrINS0_9RowVectorEEb
# 20 _ZN8facebook5velox4exec11GroupingSet8addInputERKSt10shared_ptrINS0_9RowVectorEEb
# 21 _ZN8facebook5velox4exec15HashAggregation8addInputESt10shared_ptrINS0_9RowVectorEE
# 22 _ZN8facebook5velox4exec6Driver11runInternalERSt10shared_ptrIS2_ERS3_INS1_13BlockingStateEERS3_INS0_9RowVectorEE
# 23 _ZN8facebook5velox4exec6Driver3runESt10shared_ptrIS2_E
# 24 _ZZN8facebook5velox4exec6Driver7enqueueESt10shared_ptrIS2_EENKUlvE_clEv
# 25 _ZN5folly6detail8function14FunctionTraitsIFvvEE9callSmallIZN8facebook5velox4exec6Driver7enqueueESt10shared_ptrIS9_EEUlvE_EEvRNS1_4DataE
# 26 _ZN5folly6detail8function14FunctionTraitsIFvvEEclEv
# 27 _ZN5folly18ThreadPoolExecutor7runTaskERKSt10shared_ptrINS0_6ThreadEEONS0_4TaskE
# 28 _ZN5folly21CPUThreadPoolExecutor9threadRunESt10shared_ptrINS_18ThreadPoolExecutor6ThreadEE
# 29 _ZSt13__invoke_implIvRMN5folly18ThreadPoolExecutorEFvSt10shared_ptrINS1_6ThreadEEERPS1_JRS4_EET_St21__invoke_memfun_derefOT0_OT1_DpOT2_
# 30 _ZSt8__invokeIRMN5folly18ThreadPoolExecutorEFvSt10shared_ptrINS1_6ThreadEEEJRPS1_RS4_EENSt15__invoke_resultIT_JDpT0_EE4typeEOSC_DpOSD_
# 31 _ZNSt5_BindIFMN5folly18ThreadPoolExecutorEFvSt10shared_ptrINS1_6ThreadEEEPS1_S4_EE6__callIvJEJLm0ELm1EEEET_OSt5tupleIJDpT0_EESt12_Index_tupleIJXspT1_EEE
# 32 _ZNSt5_BindIFMN5folly18ThreadPoolExecutorEFvSt10shared_ptrINS1_6ThreadEEEPS1_S4_EEclIJEvEET0_DpOT_
# 33 _ZN5folly6detail8function14FunctionTraitsIFvvEE9callSmallISt5_BindIFMNS_18ThreadPoolExecutorEFvSt10shared_ptrINS7_6ThreadEEEPS7_SA_EEEEvRNS1_4DataE
# 34 _ZN5folly6detail8function14FunctionTraitsIFvvEEclEv
# 35 _ZZN5folly18NamedThreadFactory9newThreadEONS_8FunctionIFvvEEEENUlvE_clEv
# 36 _ZSt13__invoke_implIvZN5folly18NamedThreadFactory9newThreadEONS0_8FunctionIFvvEEEEUlvE_JEET_St14__invoke_otherOT0_DpOT1_
# 37 _ZSt8__invokeIZN5folly18NamedThreadFactory9newThreadEONS0_8FunctionIFvvEEEEUlvE_JEENSt15__invoke_resultIT_JDpT0_EE4typeEOS8_DpOS9_
# 38 _ZNSt6thread8_InvokerISt5tupleIJZN5folly18NamedThreadFactory9newThreadEONS2_8FunctionIFvvEEEEUlvE_EEE9_M_invokeIJLm0EEEEvSt12_Index_tupleIJXspT_EEE
# 39 _ZNSt6thread8_InvokerISt5tupleIJZN5folly18NamedThreadFactory9newThreadEONS2_8FunctionIFvvEEEEUlvE_EEEclEv
# 40 _ZNSt6thread11_State_implINS_8_InvokerISt5tupleIJZN5folly18NamedThreadFactory9newThreadEONS3_8FunctionIFvvEEEEUlvE_EEEEE6_M_runEv
# 41 0x0000000000000000
# 42 start_thread
# 43 clone

*** Aborted at 1695293467 (Unix time, try 'date -d @1695293467') ***
*** Signal 6 (SIGABRT) (0x3052b) received by PID 197931 (pthread TID 0x7fb62545b440) (linux TID 197931) (maybe from PID 197931, UID 0) (code: -6), stack trace: ***
(error retrieving stack trace)
/bin/bash: line 8: 197931 Aborted                 (core dumped) _build/debug/velox/exec/tests/spark_aggregation_fuzzer_test --seed ${RANDOM} --duration_sec 60 --logtostderr=1 --minloglevel=0 --repro_persist_path=/tmp/spark_aggregate_fuzzer_repro

Exited with code exit status 134

[mbasmanova]

@wypb Indeed. Seems unrelated. Still, would you create a GitHub issue and add a link to failed CI job? CC: @laithsakka

[facebook-github-bot]

@mbasmanova merged this pull request in 3bb636b.

[conbench-facebook]

Conbench analyzed the 1 benchmark run on commit 3bb636b4.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

[pedroerp]

Thank you guys