You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2025/08/18 18:08:51 UTC
[PR] fix(filters): Enable decimal values in Range filter slider [superset]
rusackas opened a new pull request, #34742:
URL: https://github.com/apache/superset/pull/34742
## Summary
This PR fixes the Range filter to properly handle decimal values. Previously, the slider component defaulted to integer steps, making it impossible to select decimal values through the slider interface.
Fixes #34737
## Changes
- Added dynamic step size calculation based on the data range
- For small decimal ranges (< 1), calculates appropriate decimal precision
- For larger ranges, uses steps that provide approximately 100 increments
- Ensures step values are "nice" numbers (0.001, 0.01, 0.1, 1, 10, etc.)
- Added comprehensive tests for decimal value handling
## Test Plan
1. Create a dataset with a decimal column (e.g., values between 0.03 and 1.08)
2. Add a Range filter for that column to a dashboard
3. Verify that the slider now allows selecting decimal values
4. Test with various ranges:
- Small decimals (0.001 to 0.01)
- Medium decimals (0.03 to 1.08)
- Large ranges (0 to 1000000)
- Negative decimals (-1.5 to 2.5)
All new tests have been added and are passing:
```bash
npm run test -- src/filters/components/Range/RangeFilterPlugin.test.tsx
```
π€ Generated with [Claude Code](https://claude.ai/code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(filters): Enable decimal values in Range filter slider [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #34742:
URL: https://github.com/apache/superset/pull/34742#issuecomment-3876570402
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(filters): Enable decimal values in Range filter slider [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #34742:
URL: https://github.com/apache/superset/pull/34742#discussion_r3330211972
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.test.tsx:
##########
@@ -322,4 +322,127 @@ describe('RangeFilterPlugin', () => {
expect(sliders.length).toBeGreaterThan(0);
});
});
+
+ describe('Decimal value handling', () => {
+ it('should handle decimal ranges correctly (0.03 to 1.08)', () => {
+ const decimalProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0.03, max: 1.08 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [0.5, 0.8] },
+ };
+ getWrapper(decimalProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs).toHaveLength(2);
+ expect(inputs[0]).toHaveValue('0.5');
+ expect(inputs[1]).toHaveValue('0.8');
+
+ // Verify the slider exists and can handle decimal values
+ const sliders = screen.getAllByRole('slider');
+ expect(sliders.length).toBeGreaterThan(0);
+ });
+
+ it('should calculate appropriate step size for small decimal ranges', () => {
+ const smallRangeProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0.001, max: 0.01 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [0.005, 0.008] },
+ };
+ getWrapper(smallRangeProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs[0]).toHaveValue('0.005');
+ expect(inputs[1]).toHaveValue('0.008');
+ });
+
+ it('should handle very large ranges with appropriate step size', () => {
+ const largeRangeProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0, max: 1000000 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [100000, 500000] },
+ };
+ getWrapper(largeRangeProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs[0]).toHaveValue('100000');
+ expect(inputs[1]).toHaveValue('500000');
+ });
+
+ it('should handle negative decimal ranges', () => {
+ const negativeDecimalProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: -1.5, max: 2.5 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [-0.5, 1.5] },
+ };
+ getWrapper(negativeDecimalProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs[0]).toHaveValue('-0.5');
+ expect(inputs[1]).toHaveValue('1.5');
+ });
+
+ it('should allow decimal input via keyboard', async () => {
+ const decimalProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0, max: 10 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [null, null] },
+ };
+ getWrapper(decimalProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ const fromInput = inputs[0];
+
+ userEvent.clear(fromInput);
+ userEvent.type(fromInput, '2.5');
+ userEvent.tab();
Review Comment:
β
**Customized review instruction saved!**
**Instruction:**
> Do not flag missing `await` on `@testing-library/user-event` v12 APIs in test files when the codebase uses a synchronous v12 setup; `clear`, `type`, and `tab` do not require `await` here.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.tsx`
---
π‘ *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(filters): Enable decimal values in Range filter slider [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #34742:
URL: https://github.com/apache/superset/pull/34742#issuecomment-4626194580
**Bito Automatic Review Skipped β PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(filters): Enable decimal values in Range filter slider [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #34742:
URL: https://github.com/apache/superset/pull/34742#discussion_r3330211600
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx:
##########
@@ -234,6 +234,40 @@ export default function RangeFilterPlugin(props: PluginFilterRangeProps) {
const [row] = data;
// @ts-ignore
const { min, max }: { min: number; max: number } = row;
+
+ // Calculate appropriate step size for decimal values
+ const calculateStep = useCallback((minValue: number, maxValue: number) => {
+ const range = maxValue - minValue;
+
+ // If the range is very small (less than 1), use smaller steps
+ if (range < 1) {
+ // Find the number of decimal places needed
+ const rangeStr = range.toString();
+ const decimalMatch = rangeStr.match(/\.(\d+)/);
+ if (decimalMatch) {
+ const decimalPlaces = decimalMatch[1].length;
+ // Use a step that gives approximately 100 steps across the range
+ return Math.pow(10, -Math.min(decimalPlaces + 1, 6));
+ }
+ return 0.01;
+ }
+
+ // For larger ranges, calculate step to give approximately 100-1000 steps
+ const idealSteps = 100;
+ let step = range / idealSteps;
+
+ // Round step to a nice value (0.001, 0.01, 0.1, 1, 10, etc.)
+ const magnitude = Math.pow(10, Math.floor(Math.log10(step)));
+ step = Math.round(step / magnitude) * magnitude;
+
+ return step;
+ }, []);
Review Comment:
<!-- Bito Reply -->
The suggestion to simplify the `calculateStep` function by removing the special case for small ranges and using a unified approach with `Math.log10` and `Math.round` is valid and improves consistency. The original implementation had inconsistent logic for small vs. large ranges, which could lead to unexpected step sizes for decimal ranges. The suggested change eliminates this inconsistency by applying the same logic for all ranges, ensuring predictable behavior. This makes the function simpler and more maintainable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(filters): Enable decimal values in Range filter slider [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #34742:
URL: https://github.com/apache/superset/pull/34742#discussion_r2771970622
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx:
##########
@@ -234,6 +234,40 @@ export default function RangeFilterPlugin(props: PluginFilterRangeProps) {
const [row] = data;
// @ts-ignore
const { min, max }: { min: number; max: number } = row;
+
+ // Calculate appropriate step size for decimal values
+ const calculateStep = useCallback((minValue: number, maxValue: number) => {
+ const range = maxValue - minValue;
+
+ // If the range is very small (less than 1), use smaller steps
+ if (range < 1) {
+ // Find the number of decimal places needed
+ const rangeStr = range.toString();
+ const decimalMatch = rangeStr.match(/\.(\d+)/);
+ if (decimalMatch) {
+ const decimalPlaces = decimalMatch[1].length;
+ // Use a step that gives approximately 100 steps across the range
+ return Math.pow(10, -Math.min(decimalPlaces + 1, 6));
+ }
+ return 0.01;
+ }
+
+ // For larger ranges, calculate step to give approximately 100-1000 steps
+ const idealSteps = 100;
+ let step = range / idealSteps;
+
+ // Round step to a nice value (0.001, 0.01, 0.1, 1, 10, etc.)
+ const magnitude = Math.pow(10, Math.floor(Math.log10(step)));
+ step = Math.round(step / magnitude) * magnitude;
+
+ return step;
+ }, []);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inconsistent Step Calculation</b></div>
<div id="fix">
The calculateStep function has inconsistent logic for small vs large ranges, leading to incorrect step sizes for small decimal ranges. The special case for range < 1 does not achieve the stated goal of ~100 steps.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
- const calculateStep = useCallback((minValue: number, maxValue: number) => {
- const range = maxValue - minValue;
-
- // If the range is very small (less than 1), use smaller steps
- if (range < 1) {
- // Find the number of decimal places needed
- const rangeStr = range.toString();
- const decimalMatch = rangeStr.match(/\\.(\\d+)/);
- if (decimalMatch) {
- const decimalPlaces = decimalMatch[1].length;
- // Use a step that gives approximately 100 steps across the range
- return Math.pow(10, -Math.min(decimalPlaces + 1, 6));
- }
- return 0.01;
- }
-
- // For larger ranges, calculate step to give approximately 100-1000 steps
- const idealSteps = 100;
- let step = range / idealSteps;
-
- // Round step to a nice value (0.001, 0.01, 0.1, 1, 10, etc.)
- const magnitude = Math.pow(10, Math.floor(Math.log10(step)));
- step = Math.round(step / magnitude) * magnitude;
-
- return step;
- }, []);
+ const calculateStep = useCallback((minValue: number, maxValue: number) => {
+ const range = maxValue - minValue;
+ if (range <= 0) return 0.01;
+
+ // For all ranges, calculate step to give approximately 100 steps
+ const idealSteps = 100;
+ let step = range / idealSteps;
+
+ // Round step to a nice value (0.001, 0.01, 0.1, 1, 10, etc.)
+ const magnitude = Math.pow(10, Math.floor(Math.log10(step)));
+ step = Math.round(step / magnitude) * magnitude;
+
+ return step;
+ }, []);
```
</div>
</details>
</div>
<small><i>Code Review Run #654c72</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(filters): Enable decimal values in Range filter slider [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #34742:
URL: https://github.com/apache/superset/pull/34742#issuecomment-3876267129
### <span aria-hidden="true">β
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">π¨</span> Latest commit | 10c88c3652c327fc5bf01d17e3ded7801e232534 |
|<span aria-hidden="true">π</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/698af1cfacfca20008283a63 |
|<span aria-hidden="true">π</span> Deploy Preview | [https://deploy-preview-34742--superset-docs-preview.netlify.app](https://deploy-preview-34742--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">π±</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-34742--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(filters): Enable decimal values in Range filter slider [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #34742:
URL: https://github.com/apache/superset/pull/34742#issuecomment-3197930754
Hmm... I'm not sure if we want to go back to the concept of having the code figure out what the "steps" should be for the slider range, but this does seem like a fairly robust solution for doing so. Any thoughts on this approach @kgabryje / @kasiazjc / @michael-s-molina? I'll spin up an ephemeral when it's ready, so we can poke at it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(filters): Enable decimal values in Range filter slider [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #34742:
URL: https://github.com/apache/superset/pull/34742#issuecomment-3876242712
## Bot Review Comments Addressed
I've pushed a commit that addresses all the bot review feedback:
### Already addressed in previous commits:
- **Copilot**: Redundant `|| 0.001` fallback removed β
- **Copilot**: Changed `Math.ceil` to `Math.round` for balanced step sizes β
### Fixed in latest commit:
- **codeant-ai (test)**: Added `await` to all `userEvent` calls to prevent flaky test behavior β
- **codeant-ai (step calculation)**: Simplified `calculateStep` to avoid floating-point string parsing issues (e.g., `0.07-0.05` producing `0.020000000000000004`) β
- **bito**: Unified the step calculation logic - now uses the same consistent approach (range/100 steps with nice number rounding) for all ranges β
The simplified approach:
1. Handles all ranges consistently (small decimals, large numbers, negatives)
2. Avoids floating-point string parsing pitfalls
3. Always produces approximately 100 steps with "nice" values (0.0001, 0.001, 0.01, 0.1, 1, 10, etc.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(filters): Enable decimal values in Range filter slider [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #34742:
URL: https://github.com/apache/superset/pull/34742#issuecomment-3880340938
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(filters): Enable decimal values in Range filter slider [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #34742:
URL: https://github.com/apache/superset/pull/34742#discussion_r2286042515
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx:
##########
@@ -237,6 +237,40 @@ export default function RangeFilterPlugin(props: PluginFilterRangeProps) {
const [row] = data;
// @ts-ignore
const { min, max }: { min: number; max: number } = row;
+
+ // Calculate appropriate step size for decimal values
+ const calculateStep = useCallback((minValue: number, maxValue: number) => {
+ const range = maxValue - minValue;
+
+ // If the range is very small (less than 1), use smaller steps
+ if (range < 1) {
+ // Find the number of decimal places needed
+ const rangeStr = range.toString();
+ const decimalMatch = rangeStr.match(/\.(\d+)/);
+ if (decimalMatch) {
+ const decimalPlaces = decimalMatch[1].length;
+ // Use a step that gives approximately 100 steps across the range
+ return Math.pow(10, -Math.min(decimalPlaces + 1, 6)) || 0.001;
+ }
+ return 0.01;
+ }
+
+ // For larger ranges, calculate step to give approximately 100-1000 steps
+ const idealSteps = 100;
+ let step = range / idealSteps;
+
+ // Round step to a nice value (0.001, 0.01, 0.1, 1, 10, etc.)
+ const magnitude = Math.pow(10, Math.floor(Math.log10(step)));
+ step = Math.ceil(step / magnitude) * magnitude;
Review Comment:
Using `Math.ceil` here can produce steps that are too large. For example, if `step / magnitude` is 1.1, this becomes 2 * magnitude, doubling the intended step size. Consider using `Math.round` instead for more balanced step sizes.
```suggestion
step = Math.round(step / magnitude) * magnitude;
```
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx:
##########
@@ -237,6 +237,40 @@ export default function RangeFilterPlugin(props: PluginFilterRangeProps) {
const [row] = data;
// @ts-ignore
const { min, max }: { min: number; max: number } = row;
+
+ // Calculate appropriate step size for decimal values
+ const calculateStep = useCallback((minValue: number, maxValue: number) => {
+ const range = maxValue - minValue;
+
+ // If the range is very small (less than 1), use smaller steps
+ if (range < 1) {
+ // Find the number of decimal places needed
+ const rangeStr = range.toString();
+ const decimalMatch = rangeStr.match(/\.(\d+)/);
+ if (decimalMatch) {
+ const decimalPlaces = decimalMatch[1].length;
+ // Use a step that gives approximately 100 steps across the range
+ return Math.pow(10, -Math.min(decimalPlaces + 1, 6)) || 0.001;
Review Comment:
The fallback `|| 0.001` will never be reached because `Math.pow(10, -Math.min(decimalPlaces + 1, 6))` can never return a falsy value. Math.pow always returns a positive number for these inputs, making the fallback redundant.
```suggestion
return Math.pow(10, -Math.min(decimalPlaces + 1, 6));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(filters): Enable decimal values in Range filter slider [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #34742:
URL: https://github.com/apache/superset/pull/34742#discussion_r3358052835
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.test.tsx:
##########
@@ -320,4 +320,127 @@ describe('RangeFilterPlugin', () => {
expect(sliders.length).toBeGreaterThan(0);
});
});
+
+ describe('Decimal value handling', () => {
+ test('should handle decimal ranges correctly (0.03 to 1.08)', () => {
+ const decimalProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0.03, max: 1.08 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [0.5, 0.8] },
+ };
+ getWrapper(decimalProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs).toHaveLength(2);
+ expect(inputs[0]).toHaveValue('0.5');
+ expect(inputs[1]).toHaveValue('0.8');
+
+ // Verify the slider exists and can handle decimal values
+ const sliders = screen.getAllByRole('slider');
+ expect(sliders.length).toBeGreaterThan(0);
+ });
+
+ test('should calculate appropriate step size for small decimal ranges', () => {
+ const smallRangeProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0.001, max: 0.01 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [0.005, 0.008] },
+ };
+ getWrapper(smallRangeProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs[0]).toHaveValue('0.005');
+ expect(inputs[1]).toHaveValue('0.008');
Review Comment:
Good call β I extracted `calculateStep` into a module-level exported pure function and added direct unit tests that assert its output (integer ranges, small/large decimal ranges, the `0.05`β`0.07` floating-point case, negative/offset ranges, the tiny-range non-zero guard, and the non-positive fallback). A regression in the step calculation will now be caught directly rather than relying on input-field assertions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(filters): Enable decimal values in Range filter slider [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #34742:
URL: https://github.com/apache/superset/pull/34742#discussion_r3330283686
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.test.tsx:
##########
@@ -322,4 +322,127 @@ describe('RangeFilterPlugin', () => {
expect(sliders.length).toBeGreaterThan(0);
});
});
+
+ describe('Decimal value handling', () => {
+ it('should handle decimal ranges correctly (0.03 to 1.08)', () => {
+ const decimalProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0.03, max: 1.08 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [0.5, 0.8] },
+ };
+ getWrapper(decimalProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs).toHaveLength(2);
+ expect(inputs[0]).toHaveValue('0.5');
+ expect(inputs[1]).toHaveValue('0.8');
+
+ // Verify the slider exists and can handle decimal values
+ const sliders = screen.getAllByRole('slider');
+ expect(sliders.length).toBeGreaterThan(0);
+ });
+
+ it('should calculate appropriate step size for small decimal ranges', () => {
+ const smallRangeProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0.001, max: 0.01 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [0.005, 0.008] },
+ };
+ getWrapper(smallRangeProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs[0]).toHaveValue('0.005');
+ expect(inputs[1]).toHaveValue('0.008');
+ });
+
+ it('should handle very large ranges with appropriate step size', () => {
+ const largeRangeProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0, max: 1000000 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [100000, 500000] },
+ };
+ getWrapper(largeRangeProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs[0]).toHaveValue('100000');
+ expect(inputs[1]).toHaveValue('500000');
+ });
+
+ it('should handle negative decimal ranges', () => {
+ const negativeDecimalProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: -1.5, max: 2.5 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [-0.5, 1.5] },
+ };
+ getWrapper(negativeDecimalProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs[0]).toHaveValue('-0.5');
+ expect(inputs[1]).toHaveValue('1.5');
+ });
+
+ it('should allow decimal input via keyboard', async () => {
+ const decimalProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0, max: 10 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [null, null] },
+ };
+ getWrapper(decimalProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ const fromInput = inputs[0];
+
+ userEvent.clear(fromInput);
+ userEvent.type(fromInput, '2.5');
+ userEvent.tab();
Review Comment:
Thanks for saving the instruction.
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx:
##########
@@ -234,6 +234,40 @@ export default function RangeFilterPlugin(props: PluginFilterRangeProps) {
const [row] = data;
// @ts-ignore
const { min, max }: { min: number; max: number } = row;
+
+ // Calculate appropriate step size for decimal values
+ const calculateStep = useCallback((minValue: number, maxValue: number) => {
+ const range = maxValue - minValue;
+
+ // If the range is very small (less than 1), use smaller steps
+ if (range < 1) {
+ // Find the number of decimal places needed
+ const rangeStr = range.toString();
+ const decimalMatch = rangeStr.match(/\.(\d+)/);
+ if (decimalMatch) {
+ const decimalPlaces = decimalMatch[1].length;
+ // Use a step that gives approximately 100 steps across the range
+ return Math.pow(10, -Math.min(decimalPlaces + 1, 6));
+ }
+ return 0.01;
+ }
+
+ // For larger ranges, calculate step to give approximately 100-1000 steps
+ const idealSteps = 100;
+ let step = range / idealSteps;
+
+ // Round step to a nice value (0.001, 0.01, 0.1, 1, 10, etc.)
+ const magnitude = Math.pow(10, Math.floor(Math.log10(step)));
+ step = Math.round(step / magnitude) * magnitude;
+
+ return step;
+ }, []);
Review Comment:
Acknowledged β yes, the unified approach is already in place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(filters): Enable decimal values in Range filter slider [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #34742:
URL: https://github.com/apache/superset/pull/34742#issuecomment-3857291707
Thanks for the review feedback! I've addressed both suggestions in the latest commit:
1. **Removed the redundant `|| 0.001` fallback** - You're correct that `Math.pow(10, -Math.min(decimalPlaces + 1, 6))` can never return a falsy value. The minimum value would be `1e-7` (when decimalPlaces is 6+), which is still truthy.
2. **Changed `Math.ceil` to `Math.round`** - This provides more balanced step sizes. For example, if the ideal step is 3.2 and the magnitude is 1, `Math.ceil` would give 4 while `Math.round` gives 3, which is closer to the target of ~100 steps across the range.
The branch has also been rebased on latest master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(filters): Enable decimal values in Range filter slider [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #34742:
URL: https://github.com/apache/superset/pull/34742#discussion_r2771827378
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.test.tsx:
##########
@@ -322,4 +322,127 @@ describe('RangeFilterPlugin', () => {
expect(sliders.length).toBeGreaterThan(0);
});
});
+
+ describe('Decimal value handling', () => {
+ it('should handle decimal ranges correctly (0.03 to 1.08)', () => {
+ const decimalProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0.03, max: 1.08 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [0.5, 0.8] },
+ };
+ getWrapper(decimalProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs).toHaveLength(2);
+ expect(inputs[0]).toHaveValue('0.5');
+ expect(inputs[1]).toHaveValue('0.8');
+
+ // Verify the slider exists and can handle decimal values
+ const sliders = screen.getAllByRole('slider');
+ expect(sliders.length).toBeGreaterThan(0);
+ });
+
+ it('should calculate appropriate step size for small decimal ranges', () => {
+ const smallRangeProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0.001, max: 0.01 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [0.005, 0.008] },
+ };
+ getWrapper(smallRangeProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs[0]).toHaveValue('0.005');
+ expect(inputs[1]).toHaveValue('0.008');
+ });
+
+ it('should handle very large ranges with appropriate step size', () => {
+ const largeRangeProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0, max: 1000000 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [100000, 500000] },
+ };
+ getWrapper(largeRangeProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs[0]).toHaveValue('100000');
+ expect(inputs[1]).toHaveValue('500000');
+ });
+
+ it('should handle negative decimal ranges', () => {
+ const negativeDecimalProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: -1.5, max: 2.5 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [-0.5, 1.5] },
+ };
+ getWrapper(negativeDecimalProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs[0]).toHaveValue('-0.5');
+ expect(inputs[1]).toHaveValue('1.5');
+ });
+
+ it('should allow decimal input via keyboard', async () => {
+ const decimalProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0, max: 10 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [null, null] },
+ };
+ getWrapper(decimalProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ const fromInput = inputs[0];
+
+ userEvent.clear(fromInput);
+ userEvent.type(fromInput, '2.5');
+ userEvent.tab();
Review Comment:
**Suggestion:** The test that types a decimal value via the keyboard calls the asynchronous `userEvent.clear`, `userEvent.type`, and `userEvent.tab` functions without `await`, so the expectation on `setDataMask` can run before these interactions complete, making the test timing-dependent and potentially flaky as `@testing-library/user-event` v12 APIs are promise-based. [possible bug]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β Flaky CI test failures for RangeFilterPlugin test.
- β οΈ Slows development due to intermittent test reruns.
- β οΈ Reduces confidence in decimal input handling tests.
```
</details>
```suggestion
await userEvent.clear(fromInput);
await userEvent.type(fromInput, '2.5');
await userEvent.tab();
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Open the test file at
superset-frontend/src/filters/components/Range/RangeFilterPlugin.test.tsx and locate the
keyboard-input test starting at line 416 (`it('should allow decimal input via
keyboard'...)`). The test renders the component via `getWrapper()` and selects the first
spinbutton at lines 426-433.
2. The test performs user interactions using @testing-library/user-event at lines 434-436:
`userEvent.clear(fromInput);`, `userEvent.type(fromInput, '2.5');`, and `userEvent.tab();`
β these calls return Promises in user-event v12+ but are not awaited in the current code.
3. Immediately after the interactions, the test asserts that `setDataMask` was called with
the decimal value (expectation at lines 438-445). Because the user-event promises were not
awaited, the assertion can run before the asynchronous DOM events and associated component
handlers complete, causing the test to intermittently fail in local runs or CI.
4. Reproduce locally: run the single test (npm test -- RangeFilterPlugin.test.tsx)
repeatedly or under CI. On environments with scheduling variance, observe intermittent
failures where the expectation that `setDataMask` was called with value [2.5, null] is not
met because the typed input event handlers had not yet fired.
5. Confirm fix by changing the three userEvent calls to `await userEvent.clear(...)`,
`await userEvent.type(...)`, and `await userEvent.tab()` (as shown in the improved code).
Rerun the flaky test repeatedly; failures should disappear because the test now waits for
the user-event promises to settle before asserting.
```
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/filters/components/Range/RangeFilterPlugin.test.tsx
**Line:** 435:437
**Comment:**
*Possible Bug: The test that types a decimal value via the keyboard calls the asynchronous `userEvent.clear`, `userEvent.type`, and `userEvent.tab` functions without `await`, so the expectation on `setDataMask` can run before these interactions complete, making the test timing-dependent and potentially flaky as `@testing-library/user-event` v12 APIs are promise-based.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx:
##########
@@ -234,6 +234,40 @@ export default function RangeFilterPlugin(props: PluginFilterRangeProps) {
const [row] = data;
// @ts-ignore
const { min, max }: { min: number; max: number } = row;
+
+ // Calculate appropriate step size for decimal values
+ const calculateStep = useCallback((minValue: number, maxValue: number) => {
+ const range = maxValue - minValue;
+
+ // If the range is very small (less than 1), use smaller steps
+ if (range < 1) {
+ // Find the number of decimal places needed
+ const rangeStr = range.toString();
+ const decimalMatch = rangeStr.match(/\.(\d+)/);
+ if (decimalMatch) {
+ const decimalPlaces = decimalMatch[1].length;
Review Comment:
**Suggestion:** The decimal step calculation for small ranges relies on `range.toString()` and parsing the resulting string, which breaks for typical floating-point cases (e.g., `0.07 - 0.05` β `0.020000000000000004`) and for values rendered in scientific notation, producing either far too many steps or falling back to `0.01`βboth of which violate the "~100 steps" intent and can make the slider effectively unusable for some decimal ranges. [logic error]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β Slider resolution incorrect for common float inputs.
- β οΈ UX confusing for decimal-valued columns.
- β οΈ Affects renderSlider and user interactions.
```
</details>
```suggestion
// Find the number of decimal places based on input values rather than the range
const minDecimals = (minValue.toString().split('.')[1] || '').length;
const maxDecimals = (maxValue.toString().split('.')[1] || '').length;
const decimalPlaces = Math.max(minDecimals, maxDecimals);
if (decimalPlaces > 0) {
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Use a dataset where min and max have decimal representations that produce floating
point artifacts (e.g., min=0.05, max=0.07 -> range computed as 0.020000000000000004).
calculateStep at lines ~238-264 calls range.toString() and extracts decimals (line 245),
so the code may count many decimals because of floating-point representation.
2. On render, this inflated decimalPlaces leads to a step like 10^-(decimalPlaces+1) (line
250), which can be much smaller than intended (excessively fine step) or if parsing fails
falls back to 0.01 (line 252), both violating the "β100 steps" objective.
3. Interact with slider (renderSlider uses step={sliderStep} at lines ~585/600). Observe
either near-infinite slider resolution (tiny steps) that is impractical, or coarse
fallback (0.01) that cannot represent actual input valuesβboth break expected UX.
4. Verify by creating tests or manually logging sliderStep computed in calculateStep to
confirm mismatch with intended ~100 increments.
```
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx
**Line:** 244:248
**Comment:**
*Logic Error: The decimal step calculation for small ranges relies on `range.toString()` and parsing the resulting string, which breaks for typical floating-point cases (e.g., `0.07 - 0.05` β `0.020000000000000004`) and for values rendered in scientific notation, producing either far too many steps or falling back to `0.01`βboth of which violate the "~100 steps" intent and can make the slider effectively unusable for some decimal ranges.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(filters): Enable decimal values in Range filter slider [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34742:
URL: https://github.com/apache/superset/pull/34742#issuecomment-3339125963
β οΈ **DEPRECATED WORKFLOW** β οΈ
@rusackas This workflow is deprecated! Please use the new **Superset Showtime** system instead:
- Replace "testenv-up" label with "πͺ trigger-start"
- Better reliability and easier management
- See https://github.com/mistercrunch/superset-showtime for details
Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/18041852742). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(filters): Enable decimal values in Range filter slider [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #34742:
URL: https://github.com/apache/superset/pull/34742#discussion_r3357659874
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.test.tsx:
##########
@@ -320,4 +320,127 @@ describe('RangeFilterPlugin', () => {
expect(sliders.length).toBeGreaterThan(0);
});
});
+
+ describe('Decimal value handling', () => {
+ test('should handle decimal ranges correctly (0.03 to 1.08)', () => {
+ const decimalProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0.03, max: 1.08 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [0.5, 0.8] },
+ };
+ getWrapper(decimalProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs).toHaveLength(2);
+ expect(inputs[0]).toHaveValue('0.5');
+ expect(inputs[1]).toHaveValue('0.8');
+
+ // Verify the slider exists and can handle decimal values
+ const sliders = screen.getAllByRole('slider');
+ expect(sliders.length).toBeGreaterThan(0);
+ });
+
+ test('should calculate appropriate step size for small decimal ranges', () => {
+ const smallRangeProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0.001, max: 0.01 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [0.005, 0.008] },
+ };
+ getWrapper(smallRangeProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs[0]).toHaveValue('0.005');
+ expect(inputs[1]).toHaveValue('0.008');
Review Comment:
This test (and `should handle very large ranges with appropriate step size` below) asserts only the input-field values β it never checks the slider `step` / `calculateStep` output, which is the actual change in this PR. Both would pass unchanged on `master`, so a regression in `calculateStep` would not be caught. Consider extracting `calculateStep` and unit-testing its output directly, or asserting the rendered slider `step`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(filters): Enable decimal values in Range filter slider [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34742:
URL: https://github.com/apache/superset/pull/34742#issuecomment-3339173041
@rusackas Ephemeral environment spinning up at http://54.212.78.99:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(filters): Enable decimal values in Range filter slider [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #34742:
URL: https://github.com/apache/superset/pull/34742#issuecomment-3857523022
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(filters): Enable decimal values in Range filter slider [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #34742:
URL: https://github.com/apache/superset/pull/34742
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(filters): Enable decimal values in Range filter slider [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #34742:
URL: https://github.com/apache/superset/pull/34742#discussion_r3330211249
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx:
##########
@@ -237,6 +237,40 @@ export default function RangeFilterPlugin(props: PluginFilterRangeProps) {
const [row] = data;
// @ts-ignore
const { min, max }: { min: number; max: number } = row;
+
+ // Calculate appropriate step size for decimal values
+ const calculateStep = useCallback((minValue: number, maxValue: number) => {
+ const range = maxValue - minValue;
+
+ // If the range is very small (less than 1), use smaller steps
+ if (range < 1) {
+ // Find the number of decimal places needed
+ const rangeStr = range.toString();
+ const decimalMatch = rangeStr.match(/\.(\d+)/);
+ if (decimalMatch) {
+ const decimalPlaces = decimalMatch[1].length;
+ // Use a step that gives approximately 100 steps across the range
+ return Math.pow(10, -Math.min(decimalPlaces + 1, 6)) || 0.001;
Review Comment:
Declining β the `|| 0.0001` fallback is reachable. After `Math.round(step / magnitude) * magnitude`, `step` can be exactly 0 when `step/magnitude` is small enough to round down (e.g., range of 0.00000001 gives `step/magnitude β 0.01`, which rounds to 0). The guard prevents returning 0 to the slider, which would break it.
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx:
##########
@@ -237,6 +237,40 @@ export default function RangeFilterPlugin(props: PluginFilterRangeProps) {
const [row] = data;
// @ts-ignore
const { min, max }: { min: number; max: number } = row;
+
+ // Calculate appropriate step size for decimal values
+ const calculateStep = useCallback((minValue: number, maxValue: number) => {
+ const range = maxValue - minValue;
+
+ // If the range is very small (less than 1), use smaller steps
+ if (range < 1) {
+ // Find the number of decimal places needed
+ const rangeStr = range.toString();
+ const decimalMatch = rangeStr.match(/\.(\d+)/);
+ if (decimalMatch) {
+ const decimalPlaces = decimalMatch[1].length;
+ // Use a step that gives approximately 100 steps across the range
+ return Math.pow(10, -Math.min(decimalPlaces + 1, 6)) || 0.001;
+ }
+ return 0.01;
+ }
+
+ // For larger ranges, calculate step to give approximately 100-1000 steps
+ const idealSteps = 100;
+ let step = range / idealSteps;
+
+ // Round step to a nice value (0.001, 0.01, 0.1, 1, 10, etc.)
+ const magnitude = Math.pow(10, Math.floor(Math.log10(step)));
+ step = Math.ceil(step / magnitude) * magnitude;
Review Comment:
Already addressed β the current code uses `Math.round` (not `Math.ceil`). Agreed that `Math.round` is the right choice.
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.test.tsx:
##########
@@ -322,4 +322,127 @@ describe('RangeFilterPlugin', () => {
expect(sliders.length).toBeGreaterThan(0);
});
});
+
+ describe('Decimal value handling', () => {
+ it('should handle decimal ranges correctly (0.03 to 1.08)', () => {
+ const decimalProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0.03, max: 1.08 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [0.5, 0.8] },
+ };
+ getWrapper(decimalProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs).toHaveLength(2);
+ expect(inputs[0]).toHaveValue('0.5');
+ expect(inputs[1]).toHaveValue('0.8');
+
+ // Verify the slider exists and can handle decimal values
+ const sliders = screen.getAllByRole('slider');
+ expect(sliders.length).toBeGreaterThan(0);
+ });
+
+ it('should calculate appropriate step size for small decimal ranges', () => {
+ const smallRangeProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0.001, max: 0.01 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [0.005, 0.008] },
+ };
+ getWrapper(smallRangeProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs[0]).toHaveValue('0.005');
+ expect(inputs[1]).toHaveValue('0.008');
+ });
+
+ it('should handle very large ranges with appropriate step size', () => {
+ const largeRangeProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0, max: 1000000 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [100000, 500000] },
+ };
+ getWrapper(largeRangeProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs[0]).toHaveValue('100000');
+ expect(inputs[1]).toHaveValue('500000');
+ });
+
+ it('should handle negative decimal ranges', () => {
+ const negativeDecimalProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: -1.5, max: 2.5 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [-0.5, 1.5] },
+ };
+ getWrapper(negativeDecimalProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs[0]).toHaveValue('-0.5');
+ expect(inputs[1]).toHaveValue('1.5');
+ });
+
+ it('should allow decimal input via keyboard', async () => {
+ const decimalProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0, max: 10 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [null, null] },
+ };
+ getWrapper(decimalProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ const fromInput = inputs[0];
+
+ userEvent.clear(fromInput);
+ userEvent.type(fromInput, '2.5');
+ userEvent.tab();
Review Comment:
Not applicable here β we're on `@testing-library/user-event ^12.8.3` (per `package.json`), where `userEvent.clear/type/tab` are synchronous and don't need `await`. The separate `async` test at line 414 uses `await` as forward-compatibility hygiene; these older tests follow the synchronous v12 API.
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx:
##########
@@ -234,6 +234,40 @@ export default function RangeFilterPlugin(props: PluginFilterRangeProps) {
const [row] = data;
// @ts-ignore
const { min, max }: { min: number; max: number } = row;
+
+ // Calculate appropriate step size for decimal values
+ const calculateStep = useCallback((minValue: number, maxValue: number) => {
+ const range = maxValue - minValue;
+
+ // If the range is very small (less than 1), use smaller steps
+ if (range < 1) {
+ // Find the number of decimal places needed
+ const rangeStr = range.toString();
+ const decimalMatch = rangeStr.match(/\.(\d+)/);
+ if (decimalMatch) {
+ const decimalPlaces = decimalMatch[1].length;
Review Comment:
Already addressed β the current `calculateStep` avoids string parsing entirely and uses `Math.log10` + `Math.round` (numerically stable) for all ranges. The code comment calls this out explicitly.
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx:
##########
@@ -234,6 +234,40 @@ export default function RangeFilterPlugin(props: PluginFilterRangeProps) {
const [row] = data;
// @ts-ignore
const { min, max }: { min: number; max: number } = row;
+
+ // Calculate appropriate step size for decimal values
+ const calculateStep = useCallback((minValue: number, maxValue: number) => {
+ const range = maxValue - minValue;
+
+ // If the range is very small (less than 1), use smaller steps
+ if (range < 1) {
+ // Find the number of decimal places needed
+ const rangeStr = range.toString();
+ const decimalMatch = rangeStr.match(/\.(\d+)/);
+ if (decimalMatch) {
+ const decimalPlaces = decimalMatch[1].length;
+ // Use a step that gives approximately 100 steps across the range
+ return Math.pow(10, -Math.min(decimalPlaces + 1, 6));
+ }
+ return 0.01;
+ }
+
+ // For larger ranges, calculate step to give approximately 100-1000 steps
+ const idealSteps = 100;
+ let step = range / idealSteps;
+
+ // Round step to a nice value (0.001, 0.01, 0.1, 1, 10, etc.)
+ const magnitude = Math.pow(10, Math.floor(Math.log10(step)));
+ step = Math.round(step / magnitude) * magnitude;
+
+ return step;
+ }, []);
Review Comment:
Already addressed β `calculateStep` now uses a single `Math.log10`/`Math.round`-based approach for all ranges, eliminating the special-case inconsistency.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org