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

Stabilize Candlestick/OHLC #701

Merged
merged 4 commits into from
Feb 9, 2023
Merged

Stabilize Candlestick/OHLC #701

merged 4 commits into from
Feb 9, 2023

Conversation

thatzopoulos
Copy link
Contributor

@thatzopoulos thatzopoulos commented Feb 8, 2023

This PR renames ohlc.rs to candlestick.rs, removes the now deprecated ohlc code, and completes the following parts of Issue #695:

  • Ensure tests exist for all public API
  • Remove toolkit_experimental tags and update test usages
  • Add arrow operators for accessors if applicable
  • Ensure arrow operators have test coverage
  • If present, ensure combine and rollup are tested
  • Add serialization tests for on disk format
  • Add serialization tests for text output format
  • Add upgrade tests
  • Add continuous aggregate test

"LC_COLLATE 'C.UTF-8' LC_CTYPE 'C.UTF-8'"
}
_ => "LC_COLLATE 'C' LC_CTYPE 'C'",
match std::process::Command::new("locale").arg("-a").output() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wouldn't run on my mac when LC_CTYPE was set to UTF-8, but Smitty's match statement worked so I removed the cfg checks for target_os.

@thatzopoulos thatzopoulos marked this pull request as ready for review February 8, 2023 19:37
@thatzopoulos thatzopoulos force-pushed the th/stabilize-ohlc branch 2 times, most recently from e95a945 to da54bfb Compare February 8, 2023 19:52
extension/src/accessors.rs Outdated Show resolved Hide resolved
docs/test_candlestick_agg.md Outdated Show resolved Hide resolved
docs/test_candlestick_agg.md Outdated Show resolved Hide resolved
docs/test_candlestick_agg.md Outdated Show resolved Hide resolved
docs/test_candlestick_agg.md Outdated Show resolved Hide resolved
tests/update/candlestick.md Outdated Show resolved Hide resolved
extension/src/candlestick.rs Show resolved Hide resolved
);

let output_buffer = state.unwrap().to_pg_bytes();
let expected = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessary if we don't have time, but moving forward it would be good if we can annotate this sort of output with an explanation of what it means just to make it clear that the output is correct (see explicit_aggregate_test in frequency.rs for an example).

Copy link
Contributor

@WireBaron WireBaron left a comment

Choose a reason for hiding this comment

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

I think the only thing that needs to be fixed still is having the volume data on the cagg test.

('2023-01-11 11:12:12+00','PFE',47.38,14),
('2023-01-11 11:01:50+00','AMZN',95.25,1000),
('2023-01-11 11:01:32+00','AMZN',92,NULL),
('2023-01-11 11:01:30+00','AMZN',75.225,NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather see just a few interesting buckets (several points, data doesn't just go up and to the right) than many uninteresting ones, but this is good enough here.

docs/test_candlestick_agg.md Outdated Show resolved Hide resolved
daily_bucket | symbol | vwap | high
------------------------+--------+------+---------
2023-01-11 00:00:00+00 | AAPL | NULL | 133.445
daily_bucket | symbol | vwap | high
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not actually sure we need a rollup test here, since the cagg doesn't affect the rollup behavior at all, but if we want to have it we should look at a few more columns so we can make sure it's behaving like we expect.

@thatzopoulos
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request Feb 9, 2023
701: Stabilize Candlestick/OHLC r=thatzopoulos a=thatzopoulos

This PR renames ohlc.rs to candlestick.rs, removes the now deprecated ohlc code, and completes the following parts of Issue #695:

- [x]  Ensure tests exist for all public API
- [x]  Remove toolkit_experimental tags and update test usages
- [x]  Add arrow operators for accessors if applicable
- [x]  Ensure arrow operators have test coverage
- [x]  If present, ensure combine and rollup are tested
- [x]  Add serialization tests for on disk format
- [x]  Add serialization tests for text output format
- [x]  Add upgrade tests
- [x]  Add continuous aggregate test

Co-authored-by: Thomas Hatzopoulos <thomas@timescale.com>
@syvb

This comment was marked as resolved.

@bors

This comment was marked as resolved.

@syvb
Copy link
Member

syvb commented Feb 9, 2023

bors r-
bors r+

@bors
Copy link
Contributor

bors bot commented Feb 9, 2023

Canceled.

@bors
Copy link
Contributor

bors bot commented Feb 9, 2023

@bors bors bot merged commit d5ffff0 into main Feb 9, 2023
@bors bors bot deleted the th/stabilize-ohlc branch February 9, 2023 19:01
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