Skip to content

Conversation

@tsreaper
Copy link
Contributor

@tsreaper tsreaper commented Oct 14, 2024

Purpose

TIMESTAMP is a widely used data type. This PR supports TIMESTAMP and TIMESTAMP_LTZ type in Iceberg compatibility.

Tests

Unit tests and IT cases.

API and Format

No format changes.

Documentation

Document is also added.

| `TIMESTAMP`* | `timestamp` |
| `TIMESTAMP_LTZ`* | `timestamptz` |

*: `TIMESTAMP` and `TIMESTAMP_LTZ` type only support precision from 4 to 6
Copy link
Contributor

Choose a reason for hiding this comment

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

from 4 to 6.

+ " v_varbinary VARBINARY(20),\n"
+ " v_date DATE\n"
+ " v_date DATE,\n"
// it seems that Iceberg Flink connector has some bug when filtering a
Copy link
Contributor

Choose a reason for hiding this comment

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

Add the bug link?

case TIMESTAMP_WITHOUT_TIME_ZONE:
case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
int timestampPrecision = ((TimestampType) type).getPrecision();
long timestampLong =
Copy link
Contributor

Choose a reason for hiding this comment

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

Change the order of these two statements, for example:

Preconditions.checkArgument(
timestampPrecision > 3 && timestampPrecision <= 6,
"Paimon Iceberg compatibility only support timestamp type with precision from 4 to 6.");
long timestampLong =
ByteBuffer.wrap(bytes).order(ByteOrder.LITTLE_ENDIAN).getLong();

"decimal(%d, %d)", decimalType.getPrecision(), decimalType.getScale());
case TIMESTAMP_WITHOUT_TIME_ZONE:
int timestampPrecision = ((TimestampType) dataType).getPrecision();
Preconditions.checkArgument(
Copy link
Contributor

Choose a reason for hiding this comment

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

There are four place check this precision.
Should we extract this to reduce common code?

Copy link
Contributor

@JingsongLi JingsongLi left a comment

Choose a reason for hiding this comment

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

+1

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants