Skip to content

Arm32: Fix Datum to int cast issue#1299

Merged
svenklemm merged 1 commit intotimescale:masterfrom
Ngalstyan4:const_to_datum
Jun 21, 2019
Merged

Arm32: Fix Datum to int cast issue#1299
svenklemm merged 1 commit intotimescale:masterfrom
Ngalstyan4:const_to_datum

Conversation

@Ngalstyan4
Copy link
Copy Markdown
Contributor

@Ngalstyan4 Ngalstyan4 commented Jun 14, 2019

No description provided.

svenklemm
svenklemm requested changes Jun 14, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@svenklemm svenklemm left a comment

Choose a reason for hiding this comment

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

I dont think this is an improvement as you check for type twice now with this change

@Ngalstyan4
Copy link
Copy Markdown
Contributor Author

Ngalstyan4 commented Jun 15, 2019 •
edited
Loading

(Const*) value itself has an attribute consttype which stores its type information. This PR makes sure that we use this type information to accordingly extract value from Datum and not rely on the type information of the column.
When executing a query such as
SELECT * FROM hyper WHERE time_bucket(10, time) < 100
where time column has type bigint

the current code incorrectly assumes that the Const* type storing the value 100 is of type bigint but it does not have to be if the value could fit in a smaller type regardless of the type of the column The fix makes sure we use the type info of 100 and not that of time column in this case to extract the value from relevant Datum.

You are right that this makes an extra function call. I can inline the function or turn it into a macro if you think it would be an issue but if I understand it correctly this code runs once in the planning phase so should not add significant overhead.

The issue is very similar to this

I will add this info to the commit message. Just made a PR to get it tested on all postgres versions on ARM then remembered that I need to push to prerelease_tests for that.

@Ngalstyan4 Ngalstyan4 added the bug label Jun 15, 2019
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 15, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@1a21e41). Click here to learn what that means.
The diff coverage is 68.42%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1299   +/-   ##
=========================================
  Coverage          ?   91.85%           
=========================================
  Files             ?      111           
  Lines             ?    14451           
  Branches          ?        0           
=========================================
  Hits              ?    13274           
  Misses            ?     1177           
  Partials          ?        0
Impacted Files Coverage Δ
src/plan_expand_hypertable.c 94.69% <68.42%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a21e41...89ac865. Read the comment docs.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 15, 2019

Codecov Report

Merging #1299 into master will decrease coverage by <.01%.
The diff coverage is 68.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1299      +/-   ##
==========================================
- Coverage   91.95%   91.95%   -0.01%     
==========================================
  Files         111      111              
  Lines       14365    14372       +7     
==========================================
+ Hits        13210    13216       +6     
- Misses       1155     1156       +1
Impacted Files Coverage Δ
src/utils.h 100% <ø> (ø) ⬆️
src/plan_expand_hypertable.c 95.42% <66.66%> (ø) ⬆️
src/utils.c 84.95% <71.42%> (-0.48%) ⬇️
src/loader/bgw_message_queue.c 92.3% <0%> (+0.69%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecc9f8c...495c634. Read the comment docs.

@Ngalstyan4 Ngalstyan4 force-pushed the const_to_datum branch 4 times, most recently from e13d46f to 1525d8d Compare June 19, 2019 18:02
@Ngalstyan4 Ngalstyan4 requested a review from svenklemm June 19, 2019 23:30
svenklemm
svenklemm reviewed Jun 20, 2019
View reviewed changes
Comment thread src/plan_expand_hypertable.c Outdated
errmsg("can only use const_datum_get_int with integer types")));

Assert(false);
return 0;
Copy link
Copy Markdown
Member

@svenklemm svenklemm Jun 20, 2019

Choose a reason for hiding this comment

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

this should be pg_unreachable();

svenklemm
svenklemm approved these changes Jun 20, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@svenklemm svenklemm left a comment

Choose a reason for hiding this comment

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

LGTM

@svenklemm svenklemm added this to the 1.4.0 milestone Jun 20, 2019
@Ngalstyan4 Ngalstyan4 force-pushed the const_to_datum branch 3 times, most recently from a23b173 to abdfea3 Compare June 21, 2019 04:14
@Ngalstyan4
Fix Datum to int cast issue
89ac865 
Given a query like:

SELECT * FROM hyper WHERE time_bucket(10, time) < 100
where time column has type bigint

the current `time_bucket` parser assumes the type of (Const*)100 to
be the same as the type of the `time` column of the table.
This does not have to be the same for integer types: `time` can be a
`bigint`, but if the operand fits in an `int`, the relevant (Const*)
object will have type `int` (reflected in its `consttype` attribute).
This PR makes sure that we use this type information to accordingly
extract value from Datum and not rely on the type information of the
column.
@svenklemm svenklemm merged commit 3d63273 into timescale:master Jun 21, 2019
@cevian cevian modified the milestones: 1.4.0, 1.3.2 Jun 21, 2019
@cevian cevian changed the title Fix Datum to int cast issue Arm32: Fix Datum to int cast issue Jun 21, 2019
@Ngalstyan4 Ngalstyan4 deleted the const_to_datum branch June 22, 2019 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@svenklemm svenklemm svenklemm approved these changes

Assignees

No one assigned

Labels

bug

Projects

None yet

Milestone

1.3.2

Development

Successfully merging this pull request may close these issues.

3 participants

@Ngalstyan4 @svenklemm @cevian