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

[DISCUSSION] Reduce amount of ExprCoreTypes #1176

Closed
Yury-Fridlyand opened this issue Dec 15, 2022 · 3 comments
Closed

[DISCUSSION] Reduce amount of ExprCoreTypes #1176

Yury-Fridlyand opened this issue Dec 15, 2022 · 3 comments
Labels
maintenance Improves code quality, but not the product

Comments

@Yury-Fridlyand
Copy link
Collaborator

Is your feature request related to a problem?

  1. Do we need so much numeric ExprCoreTypes?
  2. -//- datetime ExprCoreTypes?

What solution would you like?
Why not to leave only LONG and DOUBLE or LONG/INT and FLOAT/DOUBLE?
I propose to get rid of BYTE and SHORT types, map byte and short opensearch types to INT. Thus we can reduce amount of math/comparison functions registered and simplify widening rule processing. Further removing of INT and FLOAT and using only LONG and DOUBLE would increase memory usage (for storing those values only!), but increase performance.

BYTE of even BIT/BOOL uses same amount of memory in Java (SQL/JDBC) or even in C (ODBC)(unless you use pragma pack which reduces performance). Refs: one, two.

$ java -Djdk.module.illegalAccess=deny -jar jol-cli.jar internals java.lang.Byte | grep 'Instance size'
Instance size: 16 bytes
$ java -Djdk.module.illegalAccess=deny -jar jol-cli.jar internals java.lang.Integer | grep 'Instance size'
Instance size: 16 bytes
$ java -Djdk.module.illegalAccess=deny -jar jol-cli.jar internals java.lang.Long | grep 'Instance size'
Instance size: 24 byte

Also having both DATETIME and TIMESTAMP ExprCoreTypes is a bit confusing - they contain the same information. Both types don't store time zone information.

What alternatives have you considered?
N/A

Do you have any additional context?
N/A

@Yury-Fridlyand Yury-Fridlyand added enhancement New feature or request untriaged labels Dec 15, 2022
@dai-chen dai-chen added maintenance Improves code quality, but not the product and removed enhancement New feature or request untriaged labels Dec 15, 2022
@acarbonetto
Copy link
Collaborator

Also having both DATETIME and TIMESTAMP ExprCoreTypes is a bit confusing - they contain the same information. Both types don't store time zone information.

If #126 is fixed and supports timezone, we could map to a timestamp type with timezone included. Which means we would have a reason to have both a timestamp and datatime type.

Consider not removing datetime.

@Yury-Fridlyand
Copy link
Collaborator Author

#126 is to support more date formats
This ticket to remove duplicated types.
DATETIME and TIMESTAMP contain completely the same info and adding extra complexity

@acarbonetto
Copy link
Collaborator

We can remove DATETIME, but there's no real benefit to removing BYTE/SHORT.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Improves code quality, but not the product
Projects
None yet
Development

No branches or pull requests

3 participants