Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support more type in postgres-extended #94

Merged
merged 2 commits into from
Oct 25, 2022

Conversation

ZENOTME
Copy link
Contributor

@ZENOTME ZENOTME commented Oct 22, 2022

This PR does two things:

  1. support INTERVAL && TIMESTAMPTZ type in postgres-extended
  2. refactor run() in postgres-extended: use a macro to generate process

todo!("Don't support {} type now.", column.type_().name())
}
}
generate_process!(column,row, output,idx,
Copy link
Member

Choose a reason for hiding this comment

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

Why to use macro here? How is it simplier than before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reduce duplication like

Type::NUMERIC => {
    single_process!(row, output, idx, Decimal);
}
Type::TIMESTAMP => {
    single_process!(row, output, idx, NaiveDateTime);
}
Type::DATE => {
    single_process!(row, output, idx, NaiveDate);
}
...

Copy link
Member

Choose a reason for hiding this comment

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

IMO such level of duplication is fine. Macro seems an overkill and hurts readability.

Copy link
Member

@xxchan xxchan left a comment

Choose a reason for hiding this comment

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

BTW, why would you want to format the time results instead of keeping it as-is? What's the motivation? 👀

@ZENOTME
Copy link
Contributor Author

ZENOTME commented Oct 22, 2022

BTW, why would you want to format the time results instead of keeping it as-is? What's the motivation? 👀

Because for the time type(such as INTERVAL), there is more than one string format representing the same value.
Such as '31 days' and '1 mon', they have the same semantics.
So the easiest way to compare these is to declare a fixed format.

More detail: #93

@ZENOTME ZENOTME force-pushed the support_types branch 2 times, most recently from ed3b667 to 229a726 Compare October 22, 2022 15:39
@xxchan
Copy link
Member

xxchan commented Oct 22, 2022

there is more than one string format representing the same value

But maybe some users do want to depend on such behavior, and the db implementation want to respect the behavior? (😅

@xxchan
Copy link
Member

xxchan commented Oct 22, 2022

BTW this will make postgres and postgres-extended not compatible. i.e., some test cases will pass one but fail the another. Is this what you expect?

@ZENOTME
Copy link
Contributor Author

ZENOTME commented Oct 23, 2022

BTW this will make postgres and postgres-extended not compatible. i.e., some test cases will pass one but fail the another. Is this what you expect?

In risingwave, Postgres and Postgres-extended have different e2e tests now. So I think it's acceptable to make it not compatible now. After introducing the custom comparator, we can make it compatible I think. 🤔

@xxchan
Copy link
Member

xxchan commented Oct 23, 2022

After introducing the custom comparator, we can make it compatible I think.

Actually it's still not 100% "compatible". Specifically if it can pass postgres-extended, it can fail postgres #93 (comment) 🤔

Maybe we need to completely switch to postgres-extended?

@ZENOTME
Copy link
Contributor Author

ZENOTME commented Oct 23, 2022

Maybe we need to completely switch to postgres-extended?

We can't completely switch to postgres-extended because there are some type can't support in postgres-extended such as multi-dimensional array and struct.

Copy link
Member

@skyzh skyzh left a comment

Choose a reason for hiding this comment

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

Originally I think of rewriting all of SQL queries that output date time column, so that they won't directly appear in sqllogictest. e.g, to_char(interval '15h 2m 12s', 'HH24:MI:SS'). Anyway, this approach also LGTM.

@skyzh
Copy link
Member

skyzh commented Oct 23, 2022

Ping me when you're ready to merge and release a new version.

@ZENOTME
Copy link
Contributor Author

ZENOTME commented Oct 24, 2022

Originally I think of rewriting all of SQL queries that output date time column, so that they won't directly appear in sqllogictest. e.g, to_char(interval '15h 2m 12s', 'HH24:MI:SS'). Anyway, this approach also LGTM.

I'm aware of actually we can use (...)::varchar eg: (interval '15h 2m 12s')::varchar to convert any type into varchar type so that we can compare it with string.

* support array type

Signed-off-by: Zejiong Dong <[email protected]>
@ZENOTME
Copy link
Contributor Author

ZENOTME commented Oct 25, 2022

I'm aware of actually we can use (...)::varchar eg: (interval '15h 2m 12s')::varchar to convert any type into varchar type so that we can compare it with string.

I use this way to convert theinterval timestamptztype to string. So now for these two types, Postgres and Postgres-extended are compatible. And I also do other additional work:

  • support array type of the supported single type
  • add a test file

@skyzh @xxchan

@skyzh
Copy link
Member

skyzh commented Oct 25, 2022

LGTM, merging now.

@skyzh skyzh merged commit 5fb0751 into risinglightdb:main Oct 25, 2022
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