expression: return null for empty time format by hawkingrei · Pull Request #67538 · pingcap/tidb

hawkingrei · 2026-04-02T15:46:18Z

What problem does this PR solve?

Issue Number: close #59445

Problem Summary:

TIME_FORMAT(expr, format) should return NULL when format is an empty string.

What changed and how does it work?

Return NULL early in builtinTimeFormatSig.evalString when the format string is empty.
Add a regression case in TestTimeFormat for TIME_FORMAT('12:34:56', '').

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Validation:

bazel test //pkg/expression:expression_test --test_sharding_strategy=disabled --test_arg=-test.run=^TestTimeFormat$ --test_arg=-test.v --test_output=streamed
make lint

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Fix `TIME_FORMAT(expr, '')` to return `NULL`.

Summary by CodeRabbit

Bug Fixes
- Time formatting now correctly returns NULL when given an empty format string, including in vectorized evaluation paths.
Tests
- Added unit and vectorized tests asserting TimeFormat yields NULL for an empty format string and verifying correct non-NULL formatting for valid patterns to prevent regressions.

coderabbitai · 2026-04-02T15:49:48Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 35d2f12b-5f6c-4122-954a-6f7f9e674f76

📥 Commits

Reviewing files that changed from the base of the PR and between f976add and 187e8d8.

📒 Files selected for processing (2)

pkg/expression/builtin_time_vec.go
pkg/expression/builtin_time_vec_test.go

📝 Walkthrough

Walkthrough

TIME_FORMAT now returns NULL when given an empty format string. Both scalar and vectorized evaluation paths were changed to treat "" as NULL, and unit tests (scalar and vectorized) were added to assert this behavior (issue #59445).

Changes

Cohort / File(s)	Summary
Scalar TIME_FORMAT logic `pkg/expression/builtin_time.go`	Treat empty `formatMask` as a NULL result: return `("", true, nil)` when `len(formatMask) == 0`, avoiding calling `b.formatTime` with an empty format.
Vectorized TIME_FORMAT logic `pkg/expression/builtin_time_vec.go`	When vectorized evaluation sees an empty format string (`len(buf1.GetString(i)) == 0`), append NULL and skip `formatTime`. Preserves existing NULL input checks.
Scalar tests `pkg/expression/builtin_time_test.go`	Added regression assertion in `TestTimeFormat` verifying `TIME_FORMAT('12:34:56', '')` yields NULL (issue `#59445`).
Vectorized tests `pkg/expression/builtin_time_vec_test.go`	Added `TestVectorizedTimeFormatEmptyFormatReturnsNull`: builds a vectorized function, supplies a chunk with an empty format in row 0 and `"%H:%i:%s"` in row 1, asserts NULL for row 0 and correct formatted string for row 1; verifies vectorization flags.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

qw4990
AilinKid

Poem

🐰 I nibbled the format, found an empty shell,
A gentle hop fixed the clocking spell.
Empty string now yields a NULL so neat,
Time ticks tidy on nimble feet.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title clearly and concisely describes the main change: making TIME_FORMAT return null for empty format strings.
Description check	✅ Passed	The PR description follows the template with all required sections completed: issue number, problem summary, what changed, tests checklist, side effects, and release note.
Linked Issues check	✅ Passed	The PR implementation directly addresses issue `#59445` by returning NULL when TIME_FORMAT is called with an empty format string, matching MySQL behavior. Changes were made to both scalar and vectorized implementations with corresponding tests.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing TIME_FORMAT behavior for empty format strings. No unrelated modifications or improvements are present in the changeset.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.11.4)

Command failed

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

codecov · 2026-04-02T16:12:27Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.4488%. Comparing base (c245042) to head (187e8d8).
⚠️ Report is 20 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #67538        +/-   ##
================================================
+ Coverage   77.5948%   78.4488%   +0.8539%     
================================================
  Files          1981       1974         -7     
  Lines        547950     550676      +2726     
================================================
+ Hits         425181     431999      +6818     
+ Misses       121959     118240      -3719     
+ Partials        810        437       -373

Flag	Coverage Δ
integration	`44.0164% <0.0000%> (+9.6766%)`	⬆️
unit	`77.0237% <100.0000%> (+0.6804%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`61.5065% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`49.9748% <ø> (-10.4636%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

hawkingrei · 2026-04-03T03:59:37Z

/retest

hawkingrei · 2026-04-09T05:01:56Z

/retest

windtalker · 2026-04-09T05:05:29Z

 	}
+	if len(formatMask) == 0 {
+		return "", true, nil
+	}


what about the vec version?

Good catch. I fixed the vectorized path as well and added a dedicated vectorized regression test in 187e8d8.

hawkingrei · 2026-04-09T07:48:10Z

/retest

windtalker

lgtm

ti-chi-bot · 2026-04-09T08:45:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: guo-shaoge, windtalker

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/expression/OWNERS~~ [windtalker]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2026-04-09T08:45:40Z

[LGTM Timeline notifier]

Timeline:

2026-04-09 08:23:13.210937104 +0000 UTC m=+1030998.416297162: ☑️ agreed by windtalker.
2026-04-09 08:45:39.063609479 +0000 UTC m=+1032344.268969536: ☑️ agreed by guo-shaoge.

hawkingrei · 2026-04-09T09:20:56Z

/retest

hawkingrei · 2026-04-09T10:55:45Z

/retest

hawkingrei · 2026-04-09T13:35:45Z

/retest

hawkingrei · 2026-04-09T17:49:46Z

/retest

hawkingrei · 2026-04-10T01:32:57Z

/retest

hawkingrei · 2026-04-10T03:59:51Z

/retest

hawkingrei · 2026-04-10T06:53:29Z

/retest

hawkingrei · 2026-04-10T11:52:03Z

/retest

hawkingrei · 2026-04-10T16:01:16Z

/retest

hawkingrei · 2026-04-11T07:04:01Z

/retest

hawkingrei · 2026-04-11T09:56:53Z

/retest

hawkingrei · 2026-04-11T11:13:38Z

/retest

hawkingrei · 2026-04-11T12:53:38Z

/retest

hawkingrei · 2026-04-11T18:07:25Z

/retest

hawkingrei · 2026-04-12T01:44:42Z

/retest

hawkingrei · 2026-04-12T12:51:14Z

/retest

hawkingrei · 2026-04-12T15:07:37Z

/retest

hawkingrei · 2026-04-12T18:01:38Z

/retest

hawkingrei · 2026-04-12T22:03:32Z

/retest

close pingcap#59445

ti-chi-bot Bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 2, 2026

expression: return null for empty time format

f976add

hawkingrei force-pushed the issue-59445-time-format-null branch from f286831 to f976add Compare April 8, 2026 23:24

windtalker reviewed Apr 9, 2026

View reviewed changes

pkg/expression: fix vectorized time_format empty format

187e8d8

ti-chi-bot Bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 9, 2026

windtalker approved these changes Apr 9, 2026

View reviewed changes

ti-chi-bot Bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Apr 9, 2026

hawkingrei requested a review from guo-shaoge April 9, 2026 08:27

guo-shaoge approved these changes Apr 9, 2026

View reviewed changes

ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Apr 9, 2026

ti-chi-bot Bot merged commit a83fcdb into pingcap:master Apr 12, 2026
35 checks passed

premal pushed a commit to premal/tidb that referenced this pull request Apr 30, 2026

expression: return null for empty time format (pingcap#67538)

a9774df

close pingcap#59445

Conversation

hawkingrei commented Apr 2, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

codecov Bot commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hawkingrei commented Apr 3, 2026

Uh oh!

hawkingrei commented Apr 9, 2026

Uh oh!

windtalker Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

hawkingrei Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

hawkingrei commented Apr 9, 2026

Uh oh!

windtalker left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot Bot commented Apr 9, 2026

Uh oh!

ti-chi-bot Bot commented Apr 9, 2026

[LGTM Timeline notifier]

Uh oh!

hawkingrei commented Apr 9, 2026

Uh oh!

hawkingrei commented Apr 9, 2026

Uh oh!

hawkingrei commented Apr 9, 2026

Uh oh!

hawkingrei commented Apr 9, 2026

Uh oh!

hawkingrei commented Apr 10, 2026

Uh oh!

hawkingrei commented Apr 10, 2026

Uh oh!

hawkingrei commented Apr 10, 2026

Uh oh!

hawkingrei commented Apr 10, 2026

Uh oh!

hawkingrei commented Apr 10, 2026

Uh oh!

hawkingrei commented Apr 11, 2026

Uh oh!

hawkingrei commented Apr 11, 2026

Uh oh!

hawkingrei commented Apr 11, 2026

Uh oh!

hawkingrei commented Apr 11, 2026

Uh oh!

hawkingrei commented Apr 11, 2026

Uh oh!

hawkingrei commented Apr 12, 2026

Uh oh!

hawkingrei commented Apr 12, 2026

Uh oh!

hawkingrei commented Apr 12, 2026

Uh oh!

hawkingrei commented Apr 12, 2026

Uh oh!

hawkingrei commented Apr 12, 2026

Uh oh!

hawkingrei commented Apr 2, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 2, 2026 •

edited

Loading

codecov Bot commented Apr 2, 2026 •

edited

Loading