-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Prometheus fix for count(*) #23703
base: master
Are you sure you want to change the base?
Prometheus fix for count(*) #23703
Conversation
|
@lukmanulhakkeem Please sign a CLA, its needed for the first PR - https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#contributing-to-presto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please squash all your commit to single commit and add proper commit message about the fix
prometheus.uri=http://localhost:9090 | ||
prometheus.query-chunk-duration=10m | ||
prometheus.max-query-duration=1h | ||
prometheus.uri=https://52.118.211.217:9090 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this ip here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accidentally committed test ip. I removed the line
prometheus.cache-ttl=30s | ||
#prometheus.tls.enabled=true | ||
#prometheus.tls.truststore-path=/opt/presto/etc/cert/catalog/prometheus-truststore.jks | ||
#prometheus.tls.truststore-password=changeit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these commended properties here? If we don't need it in default then should we remove these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not needed and I removed them.
83d94cc
to
94e072b
Compare
@lukmanulhakkeem Please add the Integration test as well. Please review the commit guidelines related to attribution and update the commit accordingly |
…sion of MetricHeader integration test for count(*) Signed-off-by: lukmanulhakkeem <lukmanul.hakkeem.a@ibm.com> Co-authored-by: Yuya Ebihara <ebyhry@gmail.com>
94e072b
to
d3b43c8
Compare
@agrawalreetika - I added the integration test, and in the commit message, I included the original author's name in the co-author section. Please review and let me know if any changes needed |
Description
In this Presto pull request, we are addressing the issue where a
SELECT count(*)
query returns an emptycolumnHandles
list, leading to anArrayIndexOutOfBoundsException
when the functioncolumnHandles.get(0).getColumnType()
is called. To handle this, we are bypassing the conversion frommetricHeader
to aBlock
type (performed by thegetBlockFromMap
function at line 293 inPrometheusRecordCursor.java
). We are modifying the constructor of PrometheusStandardizedRow to accept a Map type rather than a Block. The conversion to BlockFromMap is now deferred to the getFieldValue function, where getLabels() is used.Honoring the Trino PR: trinodb/trino#12510
Motivation and Context
#23702
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
If release note is NOT required, use: