Skip to content

HIVE-28075: Vectorized DayOFWeek returns inconsistent results for non-UTC timezones - #5837

Merged
okumin merged 4 commits into
apache:masterfrom
tanishq-chugh:HIVE-28075
Jun 5, 2025
Merged

okumin merged 4 commits into
apache:masterfrom
tanishq-chugh:HIVE-28075

Conversation

@tanishq-chugh

Copy link
Copy Markdown
Contributor

…-UTC timezones

What changes were proposed in this pull request?

Fix the issue of UDF dayofweek returning inconsistent results for non-UTC time zones.

Why are the changes needed?

Currently, the Calender object being used in VectorUDFDayOfWeekString.java is initialised to use UTC timezone by default. So, the date string passed to the same is represented in UTC timezone irrespective of the timezone set in hive/JVM.

Does this PR introduce any user-facing change?

No, it is fixing a bug.

How was this patch tested?

Manual. Added a qtest as well.

Comment thread ql/src/test/queries/clientpositive/udf_dayofweek_with_tz.q
date = format.parse(decoded);
Date date = DateParser.parseDate(decoded.toString());
if (date == null) {
throw new ParseException("Unable to parse date string: " + decoded, 0);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly don't know the exact security policy of Hive, and I may be overthinking it. When I examined the relevant codes, I found that no piece had exposed the argument or row. Is it possible that we should not expose decoded? Say a malicious user submits SELECT DAY_OF_WEEK(credit_card_number) FROM pii_table. A card number can be stored in HS2's log files or a client system, such as Apache Airflow. I'm considering what semantics are desirable. Related:
https://issues.apache.org/jira/browse/HIVE-20644

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this could have led to a potential CVE. i'll remove the decoded string being exposed in the exception.
Thanks for catching this @okumin !

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the change in 720ff34

@okumin okumin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a minor comment about serialVersionUID. Other than that, looks good!

*/
public final class VectorUDFDayOfWeekString extends VectorUDFTimestampFieldString {

private static final long serialVersionUID = 1L;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, please restore this variable because this class is serializable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added back in 14bd5a6

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I will merge this after the 24-hour rule is satisfied.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, Thanks @okumin !

@sonarqubecloud

sonarqubecloud Bot commented Jun 4, 2025

Copy link
Copy Markdown

@okumin okumin changed the title HIVE-28075: Vectorized DayOFWeek returns inconsistent results for non… HIVE-28075: Vectorized DayOFWeek returns inconsistent results for non-UTC timezones Jun 5, 2025
@okumin
okumin merged commit 89e7d4a into apache:master Jun 5, 2025
@okumin

okumin commented Jun 5, 2025

Copy link
Copy Markdown
Contributor

Merged. @tanishq-chugh Thanks for addressing the data integrity problem 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants