[FLINK-36941][docs] Update DATE_FORMAT Doc and Python Tests #25828

yiyutian1 · 2024-12-20T09:39:37Z

What is the purpose of the change

To update the documentation for DATE_FORMAT, which is not accurate now. The current function behaviors are:

DATE_FORMAT(timestamp, string) eventually calls formatTimestamp(TimestampData ts, String format, ZoneId zoneId) in DateTimeUtils.java, which uses DateTimeFormatter.
DATE_FORMAT(string, string) eventually calls formatTimestampStringWithOffset(String dateStr, String fromFormat, String toFormat, TimeZone tz, long offsetMills) in DateTimeUtils.java, which uses SimpleDateFormat.

Brief change log

Update documentation for DATE_FORMAT.
Add additional python tests for specific DATE_FORMAT functions.

Verifying this change

Please make sure both new and modified tests in this PR follow the conventions for tests defined in our code quality guide.

Added python tests for for specific DATE_FORMAT functions.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
The serializers: (no)
The runtime per-record code paths (performance sensitive): (no)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
The S3 file system connector: (no)

Documentation

Does this pull request introduce a new feature? (no)
If yes, how is the feature documented? (not applicable)

flinkbot · 2024-12-20T09:44:24Z

CI report:

ff312e1 Azure: FAILURE

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

snuyanzin · 2024-12-20T10:01:34Z

fyi: it can not be both [hotfix] and marked with jira issue.
So please change commit message with something like [FLINK-36941][docs] ...

gustavodemorais · 2024-12-20T10:01:57Z

Should it be [FLINK-36941][docs]?

gustavodemorais

Apart from changing the title change - lgtm!
You still need a ✅ from someone else tho ;)

davidradl · 2024-12-20T13:35:59Z

docs/data/sql_functions.yml

@@ -642,7 +642,10 @@ temporal:
    description: Returns TRUE if two time intervals defined by (timepoint1, temporal1) and (timepoint2, temporal2) overlap. The temporal values could be either a time point or a time interval. E.g., (TIME '2:55:00', INTERVAL '1' HOUR) OVERLAPS (TIME '3:30:00', INTERVAL '2' HOUR) returns TRUE; (TIME '9:00:00', TIME '10:00:00') OVERLAPS (TIME '10:15:00', INTERVAL '3' HOUR) returns FALSE.
  - sql: DATE_FORMAT(timestamp, string)
    table: dateFormat(TIMESTAMP, STRING)
-    description: Converts timestamp to a value of string in the format specified by the date format string. The format string is compatible with Java's SimpleDateFormat.
+    description: Converts timestamp to a value of string in the format specified by the date format string. The format string is compatible with Java's DateTimeFormatter.


nit: to a value of string -> to a string

davidradl · 2024-12-20T13:46:07Z

flink-python/pyflink/table/expressions.py

@@ -350,14 +350,19 @@ def temporal_overlaps(left_time_point,
 def date_format(timestamp, format) -> Expression:
    """
    Formats a timestamp as a string using a specified format.
-    The format must be compatible with MySQL's date formatting syntax as used by the
-    date_parse function.


if this has been removed - do we not need to deprecate first before replacing?

nictownsend · 2024-12-20T13:39:21Z

docs/data/sql_functions.yml

+    description: Converts timestamp to a value of string in the format specified by the date format string. The format string is compatible with Java's DateTimeFormatter.
+  - sql: DATE_FORMAT(string, string)
+    table: dateFormat(STRING, STRING)
+    description: Re-format a timestamp string to another string, using a custom format. The format string is compatible with Java's SimpleDateFormat.


Could we do the same for UNIX_TIMESTAMP(string1[, string2]) (https://github.com/apache/flink/pull/25828/files#diff-539fb22ee6aeee4cf07230bb4155500c6680c4cc889260e2c58bfa9d63fb7de5R662) please?

flink/flink-table/flink-table-common/src/main/java/org/apache/flink/table/utils/DateTimeUtils.java

Line 1468 in 7fa4f78

public static long unixTimestamp(String dateStr, String format, TimeZone tz) {

calls into

flink/flink-table/flink-table-common/src/main/java/org/apache/flink/table/utils/DateTimeUtils.java

Line 960 in 7fa4f78

private static long internalParseTimestampMillis(String dateStr, String format, TimeZone tz) {

too - so the format is also using SimpleDateFormat

Assuming I've read

flink/flink-table/flink-table-common/src/main/java/org/apache/flink/table/functions/BuiltInFunctionDefinitions.java

Line 2304 in 7fa4f78

public static final BuiltInFunctionDefinition UNIX_TIMESTAMP =

to mean it invokes unixTimestamp(dateStr, format, null) as there is no tz third parameter in the SQL function.

@nictownsend sounds like a reasonable idea, @snuyanzin @yiyutian1 if you agree we could raise this separately.

Apologies, I hadn't appreciated the scope of the underlying Flink issue, happy to raise a new one if it's not appropriate to fix here

I have a slightly different thought
Since it is Flink 2.0 and anyway it brings some breaking changes. May be we can completely switch to DateTimeFormatter in 2.0 while for 1.19 and 1.20 we just fix documentation.
However this option requires a discussion in ML
i can start one if no objections

@snuyanzin I like the idea.
Before that happens, should we fix the current doc?
If to only commit for 1.19 and 1.20, should I create PR against release-1.19 and release-1.20?

davidradl · 2024-12-20T13:47:18Z

flink-python/pyflink/table/expressions.py

-    For example `date_format(col("time"), "%Y, %d %M")` results in strings formatted as
-    "2017, 05 May".
+    Supported functions:
+    1. date_format(TIMESTAMP, STRING) -> STRING


we should mention the difference between the supported format strings.

davidradl · 2024-12-20T14:03:00Z

flink-python/pyflink/table/tests/test_expression.py

@@ -299,6 +299,10 @@ def test_expressions(self):
                             lit(2).hours)))
        self.assertEqual("dateFormat(time, '%Y, %d %M')",
                         str(date_format(col("time"), "%Y, %d %M")))
+        self.assertEqual("dateFormat(toTimestamp('1970-01-01 08:01:40'), 'yyyy-MM-dd HH:mm:ss')",
+                         str(date_format(to_timestamp('1970-01-01 08:01:40'), "yyyy-MM-dd HH:mm:ss")))
+        self.assertEqual("dateFormat('1970-01-01 08:01:40', 'yyyy-MM-dd HH:mm:ss')",


could you add a test with the format that the DateTimeFormatter could cope with - with precision and timezone

Hi @davidradl , sure I can add that.
Also want to point out that these test_expressions don't test any implementations, they just test the "signature" of defined functions.

davidradl · 2024-12-20T14:04:27Z

docs/data/sql_functions.yml

@@ -642,7 +642,10 @@ temporal:
    description: Returns TRUE if two time intervals defined by (timepoint1, temporal1) and (timepoint2, temporal2) overlap. The temporal values could be either a time point or a time interval. E.g., (TIME '2:55:00', INTERVAL '1' HOUR) OVERLAPS (TIME '3:30:00', INTERVAL '2' HOUR) returns TRUE; (TIME '9:00:00', TIME '10:00:00') OVERLAPS (TIME '10:15:00', INTERVAL '3' HOUR) returns FALSE.
  - sql: DATE_FORMAT(timestamp, string)
    table: dateFormat(TIMESTAMP, STRING)
-    description: Converts timestamp to a value of string in the format specified by the date format string. The format string is compatible with Java's SimpleDateFormat.
+    description: Converts timestamp to a value of string in the format specified by the date format string. The format string is compatible with Java's DateTimeFormatter.


I am curious what happens if you supply a TIMESTAMP_TZ into this function, is this expected to work?

yiyutian1 marked this pull request as ready for review December 20, 2024 09:42

yiyutian1 force-pushed the flink-36941 branch from 5a9ea55 to 8e32861 Compare December 20, 2024 09:49

gustavodemorais approved these changes Dec 20, 2024

View reviewed changes

flinkbot added the component=Documentation label Dec 20, 2024

davidradl reviewed Dec 20, 2024

View reviewed changes

nictownsend reviewed Dec 20, 2024

View reviewed changes

davidradl reviewed Dec 20, 2024

View reviewed changes

yiyutian1 changed the title ~~[FLINK-36941][hotfix] Update DATE_FORMAT Doc and Python Tests~~ [FLINK-36941][docs] Update DATE_FORMAT Doc and Python Tests Dec 21, 2024

[FLINK-36941][docs] Update DATE_FORMAT Doc and Python Tests

ff312e1

yiyutian1 force-pushed the flink-36941 branch from 8e32861 to ff312e1 Compare December 21, 2024 00:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-36941][docs] Update DATE_FORMAT Doc and Python Tests #25828

[FLINK-36941][docs] Update DATE_FORMAT Doc and Python Tests #25828

yiyutian1 commented Dec 20, 2024 •

edited

Loading

flinkbot commented Dec 20, 2024 •

edited

Loading

snuyanzin commented Dec 20, 2024

gustavodemorais commented Dec 20, 2024

gustavodemorais left a comment

davidradl Dec 20, 2024

davidradl Dec 20, 2024

nictownsend Dec 20, 2024

nictownsend Dec 20, 2024

davidradl Dec 20, 2024

nictownsend Dec 20, 2024

snuyanzin Dec 20, 2024 •

edited

Loading

yiyutian1 Dec 21, 2024

davidradl Dec 20, 2024

davidradl Dec 20, 2024

yiyutian1 Dec 21, 2024

davidradl Dec 20, 2024

[FLINK-36941][docs] Update DATE_FORMAT Doc and Python Tests #25828

Are you sure you want to change the base?

[FLINK-36941][docs] Update DATE_FORMAT Doc and Python Tests #25828

Conversation

yiyutian1 commented Dec 20, 2024 • edited Loading

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Dec 20, 2024 • edited Loading

CI report:

snuyanzin commented Dec 20, 2024

gustavodemorais commented Dec 20, 2024

gustavodemorais left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snuyanzin Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yiyutian1 commented Dec 20, 2024 •

edited

Loading

flinkbot commented Dec 20, 2024 •

edited

Loading

snuyanzin Dec 20, 2024 •

edited

Loading