bito-code-review[bot] commented on code in PR #35033:
URL: https://github.com/apache/superset/pull/35033#discussion_r2326686501
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/utils.test.ts:
##########
@@ -44,3 +44,450 @@ describe('getColorBreakpointsBuckets', () => {
expect(result).toEqual({});
});
});
+
+describe('getBreakPoints', () => {
+ const accessor = (d: any) => d.value;
+
+ describe('automatic breakpoint generation', () => {
+ it('generates correct number of breakpoints for given buckets', () => {
+ const features = [
+ { value: 0 },
+ { value: 50 },
+ { value: 100 },
+ ];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '5' },
+ features,
+ accessor,
+ );
+
+ expect(breakPoints).toHaveLength(6); // n buckets = n+1 breakpoints
+ expect(breakPoints.every(bp => typeof bp === 'string')).toBe(true);
+ });
+
+ it('ensures data range is fully covered', () => {
+ // Test various data ranges to ensure min/max are always included
+ const testCases = [
+ { data: [0, 100], buckets: 5 },
+ { data: [0.1, 99.9], buckets: 4 },
+ { data: [-50, 50], buckets: 10 },
+ { data: [3.2, 38.7], buckets: 5 }, // Original max bug case
+ { data: [3.14, 100], buckets: 5 }, // Min rounding bug case (3.14 -> 3)
+ { data: [2.345, 10], buckets: 4 }, // Min rounding bug case (2.345 ->
2.35)
+ { data: [0.0001, 0.0009], buckets: 3 }, // Very small numbers
+ { data: [1000000, 9000000], buckets: 8 }, // Large numbers
+ ];
+
+ testCases.forEach(({ data, buckets }) => {
+ const [min, max] = data;
+ const features = [{ value: min }, { value: max }];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: String(buckets) },
+ features,
+ accessor,
+ );
+
+ const firstBp = parseFloat(breakPoints[0]);
+ const lastBp = parseFloat(breakPoints[breakPoints.length - 1]);
+
+ // Critical: min and max must be within the breakpoint range
+ expect(firstBp).toBeLessThanOrEqual(min);
+ expect(lastBp).toBeGreaterThanOrEqual(max);
+ expect(breakPoints).toHaveLength(buckets + 1);
+ });
+ });
+
+ it('handles uniform distribution correctly', () => {
+ const features = [
+ { value: 0 },
+ { value: 25 },
+ { value: 50 },
+ { value: 75 },
+ { value: 100 },
+ ];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '4' },
+ features,
+ accessor,
+ );
+
+ // Check that breakpoints are evenly spaced
+ const numericBreakPoints = breakPoints.map(parseFloat);
+ const deltas = [];
+ for (let i = 1; i < numericBreakPoints.length; i++) {
+ deltas.push(numericBreakPoints[i] - numericBreakPoints[i - 1]);
+ }
+
+ // All deltas should be approximately equal
+ const avgDelta = deltas.reduce((a, b) => a + b, 0) / deltas.length;
+ deltas.forEach(delta => {
+ expect(delta).toBeCloseTo(avgDelta, 1);
+ });
+ });
+
+ it('handles single value datasets', () => {
+ const features = [
+ { value: 42 },
+ { value: 42 },
+ { value: 42 },
+ ];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '5' },
+ features,
+ accessor,
+ );
+
+ const firstBp = parseFloat(breakPoints[0]);
+ const lastBp = parseFloat(breakPoints[breakPoints.length - 1]);
+
+ expect(firstBp).toBeLessThanOrEqual(42);
+ expect(lastBp).toBeGreaterThanOrEqual(42);
+ });
+
+ it('preserves appropriate precision for different scales', () => {
+ const testCases = [
+ { data: [0, 1], expectedMaxPrecision: 1 }, // 0.0, 0.2, 0.4...
+ { data: [0, 0.1], expectedMaxPrecision: 2 }, // 0.00, 0.02...
+ { data: [0, 0.01], expectedMaxPrecision: 3 }, // 0.000, 0.002...
+ { data: [0, 1000], expectedMaxPrecision: 0 }, // 0, 200, 400...
+ ];
+
+ testCases.forEach(({ data, expectedMaxPrecision }) => {
+ const [min, max] = data;
+ const features = [{ value: min }, { value: max }];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '5' },
+ features,
+ accessor,
+ );
+
+ breakPoints.forEach(bp => {
+ const decimalPlaces = (bp.split('.')[1] || '').length;
+ expect(decimalPlaces).toBeLessThanOrEqual(expectedMaxPrecision);
+ });
+ });
+ });
+
+ it('handles negative values correctly', () => {
+ const features = [
+ { value: -100 },
+ { value: -50 },
+ { value: 0 },
+ { value: 50 },
+ { value: 100 },
+ ];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '5' },
+ features,
+ accessor,
+ );
+
+ const numericBreakPoints = breakPoints.map(parseFloat);
+ expect(numericBreakPoints[0]).toBeLessThanOrEqual(-100);
+ expect(numericBreakPoints[numericBreakPoints.length -
1]).toBeGreaterThanOrEqual(100);
+
+ // Verify ascending order
+ for (let i = 1; i < numericBreakPoints.length; i++) {
+ expect(numericBreakPoints[i]).toBeGreaterThan(numericBreakPoints[i -
1]);
+ }
+ });
+
+ it('handles mixed integer and decimal values', () => {
+ const features = [
+ { value: 1 },
+ { value: 2.5 },
+ { value: 3.7 },
+ { value: 5 },
+ { value: 8.2 },
+ ];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '4' },
+ features,
+ accessor,
+ );
+
+ const firstBp = parseFloat(breakPoints[0]);
+ const lastBp = parseFloat(breakPoints[breakPoints.length - 1]);
+
+ expect(firstBp).toBeLessThanOrEqual(1);
+ expect(lastBp).toBeGreaterThanOrEqual(8.2);
+ });
+
+ it('uses floor/ceil for boundary breakpoints to ensure inclusion', () => {
+ // Test that Math.floor and Math.ceil are used for boundaries
+ // This ensures all data points fall within the breakpoint range
+
+ const testCases = [
+ { minValue: 3.14, maxValue: 100, buckets: 5 },
+ { minValue: 2.345, maxValue: 10.678, buckets: 4 },
+ { minValue: 1.67, maxValue: 5.33, buckets: 3 },
+ { minValue: 0.123, maxValue: 0.987, buckets: 5 },
+ ];
+
+ testCases.forEach(({ minValue, maxValue, buckets }) => {
+ const features = [{ value: minValue }, { value: maxValue }];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: String(buckets) },
+ features,
+ accessor,
+ );
+
+ const firstBp = parseFloat(breakPoints[0]);
+ const lastBp = parseFloat(breakPoints[breakPoints.length - 1]);
+
+ // First breakpoint should be floored (always <= minValue)
+ expect(firstBp).toBeLessThanOrEqual(minValue);
+
+ // Last breakpoint should be ceiled (always >= maxValue)
+ expect(lastBp).toBeGreaterThanOrEqual(maxValue);
+
+ // All values should be within range
+ expect(minValue).toBeGreaterThanOrEqual(firstBp);
+ expect(maxValue).toBeLessThanOrEqual(lastBp);
+ });
+ });
+
+ it('prevents minimum value exclusion edge case', () => {
+ // Specific edge case test for minimum value exclusion
+ // Tests the exact scenario where rounding would exclude the min value
+
+ const features = [
+ { value: 3.14 }, // This would round to 3 at precision 0
+ { value: 50 },
+ { value: 100 },
+ ];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '5' },
+ features,
+ accessor,
+ );
+
+ const firstBp = parseFloat(breakPoints[0]);
+
+ // The first breakpoint must be <= 3.14 (floor behavior)
+ expect(firstBp).toBeLessThanOrEqual(3.14);
+
+ // Verify that 3.14 is not excluded
+ expect(3.14).toBeGreaterThanOrEqual(firstBp);
+
+ // The first breakpoint should be a clean floor value
+ expect(breakPoints[0]).toMatch(/^3(\.0+)?$/);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect regex pattern for number format</b></div>
<div id="fix">
The regex pattern `/^3(\.0+)?$/` may fail to match the actual output from
`toFixed(precision)`. When precision is 0, `toFixed()` returns '3' without
decimal places, but the regex requires at least one zero after the decimal
point. Change to `/^3(\.0*)?$/` to allow zero or more decimal zeros.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
- expect(breakPoints[0]).toMatch(/^3(\\.0+)?$/);
+ expect(breakPoints[0]).toMatch(/^3(\\.0*)?$/);
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/35033#issuecomment-3261658269>#3ebed4</a></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
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/utils.test.ts:
##########
@@ -44,3 +44,450 @@ describe('getColorBreakpointsBuckets', () => {
expect(result).toEqual({});
});
});
+
+describe('getBreakPoints', () => {
+ const accessor = (d: any) => d.value;
+
+ describe('automatic breakpoint generation', () => {
+ it('generates correct number of breakpoints for given buckets', () => {
+ const features = [
+ { value: 0 },
+ { value: 50 },
+ { value: 100 },
+ ];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '5' },
+ features,
+ accessor,
+ );
+
+ expect(breakPoints).toHaveLength(6); // n buckets = n+1 breakpoints
+ expect(breakPoints.every(bp => typeof bp === 'string')).toBe(true);
+ });
+
+ it('ensures data range is fully covered', () => {
+ // Test various data ranges to ensure min/max are always included
+ const testCases = [
+ { data: [0, 100], buckets: 5 },
+ { data: [0.1, 99.9], buckets: 4 },
+ { data: [-50, 50], buckets: 10 },
+ { data: [3.2, 38.7], buckets: 5 }, // Original max bug case
+ { data: [3.14, 100], buckets: 5 }, // Min rounding bug case (3.14 -> 3)
+ { data: [2.345, 10], buckets: 4 }, // Min rounding bug case (2.345 ->
2.35)
+ { data: [0.0001, 0.0009], buckets: 3 }, // Very small numbers
+ { data: [1000000, 9000000], buckets: 8 }, // Large numbers
+ ];
+
+ testCases.forEach(({ data, buckets }) => {
+ const [min, max] = data;
+ const features = [{ value: min }, { value: max }];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: String(buckets) },
+ features,
+ accessor,
+ );
+
+ const firstBp = parseFloat(breakPoints[0]);
+ const lastBp = parseFloat(breakPoints[breakPoints.length - 1]);
+
+ // Critical: min and max must be within the breakpoint range
+ expect(firstBp).toBeLessThanOrEqual(min);
+ expect(lastBp).toBeGreaterThanOrEqual(max);
+ expect(breakPoints).toHaveLength(buckets + 1);
+ });
+ });
+
+ it('handles uniform distribution correctly', () => {
+ const features = [
+ { value: 0 },
+ { value: 25 },
+ { value: 50 },
+ { value: 75 },
+ { value: 100 },
+ ];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '4' },
+ features,
+ accessor,
+ );
+
+ // Check that breakpoints are evenly spaced
+ const numericBreakPoints = breakPoints.map(parseFloat);
+ const deltas = [];
+ for (let i = 1; i < numericBreakPoints.length; i++) {
+ deltas.push(numericBreakPoints[i] - numericBreakPoints[i - 1]);
+ }
+
+ // All deltas should be approximately equal
+ const avgDelta = deltas.reduce((a, b) => a + b, 0) / deltas.length;
+ deltas.forEach(delta => {
+ expect(delta).toBeCloseTo(avgDelta, 1);
+ });
+ });
+
+ it('handles single value datasets', () => {
+ const features = [
+ { value: 42 },
+ { value: 42 },
+ { value: 42 },
+ ];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '5' },
+ features,
+ accessor,
+ );
+
+ const firstBp = parseFloat(breakPoints[0]);
+ const lastBp = parseFloat(breakPoints[breakPoints.length - 1]);
+
+ expect(firstBp).toBeLessThanOrEqual(42);
+ expect(lastBp).toBeGreaterThanOrEqual(42);
+ });
+
+ it('preserves appropriate precision for different scales', () => {
+ const testCases = [
+ { data: [0, 1], expectedMaxPrecision: 1 }, // 0.0, 0.2, 0.4...
+ { data: [0, 0.1], expectedMaxPrecision: 2 }, // 0.00, 0.02...
+ { data: [0, 0.01], expectedMaxPrecision: 3 }, // 0.000, 0.002...
+ { data: [0, 1000], expectedMaxPrecision: 0 }, // 0, 200, 400...
+ ];
+
+ testCases.forEach(({ data, expectedMaxPrecision }) => {
+ const [min, max] = data;
+ const features = [{ value: min }, { value: max }];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '5' },
+ features,
+ accessor,
+ );
+
+ breakPoints.forEach(bp => {
+ const decimalPlaces = (bp.split('.')[1] || '').length;
+ expect(decimalPlaces).toBeLessThanOrEqual(expectedMaxPrecision);
+ });
+ });
+ });
+
+ it('handles negative values correctly', () => {
+ const features = [
+ { value: -100 },
+ { value: -50 },
+ { value: 0 },
+ { value: 50 },
+ { value: 100 },
+ ];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '5' },
+ features,
+ accessor,
+ );
+
+ const numericBreakPoints = breakPoints.map(parseFloat);
+ expect(numericBreakPoints[0]).toBeLessThanOrEqual(-100);
+ expect(numericBreakPoints[numericBreakPoints.length -
1]).toBeGreaterThanOrEqual(100);
+
+ // Verify ascending order
+ for (let i = 1; i < numericBreakPoints.length; i++) {
+ expect(numericBreakPoints[i]).toBeGreaterThan(numericBreakPoints[i -
1]);
+ }
+ });
+
+ it('handles mixed integer and decimal values', () => {
+ const features = [
+ { value: 1 },
+ { value: 2.5 },
+ { value: 3.7 },
+ { value: 5 },
+ { value: 8.2 },
+ ];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '4' },
+ features,
+ accessor,
+ );
+
+ const firstBp = parseFloat(breakPoints[0]);
+ const lastBp = parseFloat(breakPoints[breakPoints.length - 1]);
+
+ expect(firstBp).toBeLessThanOrEqual(1);
+ expect(lastBp).toBeGreaterThanOrEqual(8.2);
+ });
+
+ it('uses floor/ceil for boundary breakpoints to ensure inclusion', () => {
+ // Test that Math.floor and Math.ceil are used for boundaries
+ // This ensures all data points fall within the breakpoint range
+
+ const testCases = [
+ { minValue: 3.14, maxValue: 100, buckets: 5 },
+ { minValue: 2.345, maxValue: 10.678, buckets: 4 },
+ { minValue: 1.67, maxValue: 5.33, buckets: 3 },
+ { minValue: 0.123, maxValue: 0.987, buckets: 5 },
+ ];
+
+ testCases.forEach(({ minValue, maxValue, buckets }) => {
+ const features = [{ value: minValue }, { value: maxValue }];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: String(buckets) },
+ features,
+ accessor,
+ );
+
+ const firstBp = parseFloat(breakPoints[0]);
+ const lastBp = parseFloat(breakPoints[breakPoints.length - 1]);
+
+ // First breakpoint should be floored (always <= minValue)
+ expect(firstBp).toBeLessThanOrEqual(minValue);
+
+ // Last breakpoint should be ceiled (always >= maxValue)
+ expect(lastBp).toBeGreaterThanOrEqual(maxValue);
+
+ // All values should be within range
+ expect(minValue).toBeGreaterThanOrEqual(firstBp);
+ expect(maxValue).toBeLessThanOrEqual(lastBp);
+ });
+ });
+
+ it('prevents minimum value exclusion edge case', () => {
+ // Specific edge case test for minimum value exclusion
+ // Tests the exact scenario where rounding would exclude the min value
+
+ const features = [
+ { value: 3.14 }, // This would round to 3 at precision 0
+ { value: 50 },
+ { value: 100 },
+ ];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '5' },
+ features,
+ accessor,
+ );
+
+ const firstBp = parseFloat(breakPoints[0]);
+
+ // The first breakpoint must be <= 3.14 (floor behavior)
+ expect(firstBp).toBeLessThanOrEqual(3.14);
+
+ // Verify that 3.14 is not excluded
+ expect(3.14).toBeGreaterThanOrEqual(firstBp);
+
+ // The first breakpoint should be a clean floor value
+ expect(breakPoints[0]).toMatch(/^3(\.0+)?$/);
+ });
+
+ it('prevents maximum value exclusion edge case', () => {
+ // Specific edge case test for maximum value exclusion
+ // Tests the exact scenario where rounding would exclude the max value
+
+ const features = [
+ { value: 0 },
+ { value: 20 },
+ { value: 38.7 }, // Original bug case
+ ];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '5' },
+ features,
+ accessor,
+ );
+
+ const lastBp = parseFloat(breakPoints[breakPoints.length - 1]);
+
+ // The last breakpoint must be >= 38.7 (ceil behavior)
+ expect(lastBp).toBeGreaterThanOrEqual(38.7);
+
+ // Verify that 38.7 is not excluded
+ expect(38.7).toBeLessThanOrEqual(lastBp);
+
+ // The last breakpoint should be a clean ceil value
+ expect(breakPoints[breakPoints.length - 1]).toMatch(/^39(\.0+)?$/);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect regex pattern for number format</b></div>
<div id="fix">
The regex pattern `/^39(\.0+)?$/` may fail to match the actual output from
`toFixed(precision)`. When precision is 0, `toFixed()` returns '39' without
decimal places, but the regex requires at least one zero after the decimal
point. Change to `/^39(\.0*)?$/` to allow zero or more decimal zeros.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
- expect(breakPoints[breakPoints.length - 1]).toMatch(/^39(\\.0+)?$/);
+ expect(breakPoints[breakPoints.length - 1]).toMatch(/^39(\\.0*)?$/);
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/35033#issuecomment-3261658269>#3ebed4</a></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
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/utils.test.ts:
##########
@@ -44,3 +44,450 @@ describe('getColorBreakpointsBuckets', () => {
expect(result).toEqual({});
});
});
+
+describe('getBreakPoints', () => {
+ const accessor = (d: any) => d.value;
+
+ describe('automatic breakpoint generation', () => {
+ it('generates correct number of breakpoints for given buckets', () => {
+ const features = [
+ { value: 0 },
+ { value: 50 },
+ { value: 100 },
+ ];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '5' },
+ features,
+ accessor,
+ );
+
+ expect(breakPoints).toHaveLength(6); // n buckets = n+1 breakpoints
+ expect(breakPoints.every(bp => typeof bp === 'string')).toBe(true);
+ });
+
+ it('ensures data range is fully covered', () => {
+ // Test various data ranges to ensure min/max are always included
+ const testCases = [
+ { data: [0, 100], buckets: 5 },
+ { data: [0.1, 99.9], buckets: 4 },
+ { data: [-50, 50], buckets: 10 },
+ { data: [3.2, 38.7], buckets: 5 }, // Original max bug case
+ { data: [3.14, 100], buckets: 5 }, // Min rounding bug case (3.14 -> 3)
+ { data: [2.345, 10], buckets: 4 }, // Min rounding bug case (2.345 ->
2.35)
+ { data: [0.0001, 0.0009], buckets: 3 }, // Very small numbers
+ { data: [1000000, 9000000], buckets: 8 }, // Large numbers
+ ];
+
+ testCases.forEach(({ data, buckets }) => {
+ const [min, max] = data;
+ const features = [{ value: min }, { value: max }];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: String(buckets) },
+ features,
+ accessor,
+ );
+
+ const firstBp = parseFloat(breakPoints[0]);
+ const lastBp = parseFloat(breakPoints[breakPoints.length - 1]);
+
+ // Critical: min and max must be within the breakpoint range
+ expect(firstBp).toBeLessThanOrEqual(min);
+ expect(lastBp).toBeGreaterThanOrEqual(max);
+ expect(breakPoints).toHaveLength(buckets + 1);
+ });
+ });
+
+ it('handles uniform distribution correctly', () => {
+ const features = [
+ { value: 0 },
+ { value: 25 },
+ { value: 50 },
+ { value: 75 },
+ { value: 100 },
+ ];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '4' },
+ features,
+ accessor,
+ );
+
+ // Check that breakpoints are evenly spaced
+ const numericBreakPoints = breakPoints.map(parseFloat);
+ const deltas = [];
+ for (let i = 1; i < numericBreakPoints.length; i++) {
+ deltas.push(numericBreakPoints[i] - numericBreakPoints[i - 1]);
+ }
+
+ // All deltas should be approximately equal
+ const avgDelta = deltas.reduce((a, b) => a + b, 0) / deltas.length;
+ deltas.forEach(delta => {
+ expect(delta).toBeCloseTo(avgDelta, 1);
+ });
+ });
+
+ it('handles single value datasets', () => {
+ const features = [
+ { value: 42 },
+ { value: 42 },
+ { value: 42 },
+ ];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '5' },
+ features,
+ accessor,
+ );
+
+ const firstBp = parseFloat(breakPoints[0]);
+ const lastBp = parseFloat(breakPoints[breakPoints.length - 1]);
+
+ expect(firstBp).toBeLessThanOrEqual(42);
+ expect(lastBp).toBeGreaterThanOrEqual(42);
+ });
+
+ it('preserves appropriate precision for different scales', () => {
+ const testCases = [
+ { data: [0, 1], expectedMaxPrecision: 1 }, // 0.0, 0.2, 0.4...
+ { data: [0, 0.1], expectedMaxPrecision: 2 }, // 0.00, 0.02...
+ { data: [0, 0.01], expectedMaxPrecision: 3 }, // 0.000, 0.002...
+ { data: [0, 1000], expectedMaxPrecision: 0 }, // 0, 200, 400...
+ ];
+
+ testCases.forEach(({ data, expectedMaxPrecision }) => {
+ const [min, max] = data;
+ const features = [{ value: min }, { value: max }];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '5' },
+ features,
+ accessor,
+ );
+
+ breakPoints.forEach(bp => {
+ const decimalPlaces = (bp.split('.')[1] || '').length;
+ expect(decimalPlaces).toBeLessThanOrEqual(expectedMaxPrecision);
+ });
+ });
+ });
+
+ it('handles negative values correctly', () => {
+ const features = [
+ { value: -100 },
+ { value: -50 },
+ { value: 0 },
+ { value: 50 },
+ { value: 100 },
+ ];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '5' },
+ features,
+ accessor,
+ );
+
+ const numericBreakPoints = breakPoints.map(parseFloat);
+ expect(numericBreakPoints[0]).toBeLessThanOrEqual(-100);
+ expect(numericBreakPoints[numericBreakPoints.length -
1]).toBeGreaterThanOrEqual(100);
+
+ // Verify ascending order
+ for (let i = 1; i < numericBreakPoints.length; i++) {
+ expect(numericBreakPoints[i]).toBeGreaterThan(numericBreakPoints[i -
1]);
+ }
+ });
+
+ it('handles mixed integer and decimal values', () => {
+ const features = [
+ { value: 1 },
+ { value: 2.5 },
+ { value: 3.7 },
+ { value: 5 },
+ { value: 8.2 },
+ ];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '4' },
+ features,
+ accessor,
+ );
+
+ const firstBp = parseFloat(breakPoints[0]);
+ const lastBp = parseFloat(breakPoints[breakPoints.length - 1]);
+
+ expect(firstBp).toBeLessThanOrEqual(1);
+ expect(lastBp).toBeGreaterThanOrEqual(8.2);
+ });
+
+ it('uses floor/ceil for boundary breakpoints to ensure inclusion', () => {
+ // Test that Math.floor and Math.ceil are used for boundaries
+ // This ensures all data points fall within the breakpoint range
+
+ const testCases = [
+ { minValue: 3.14, maxValue: 100, buckets: 5 },
+ { minValue: 2.345, maxValue: 10.678, buckets: 4 },
+ { minValue: 1.67, maxValue: 5.33, buckets: 3 },
+ { minValue: 0.123, maxValue: 0.987, buckets: 5 },
+ ];
+
+ testCases.forEach(({ minValue, maxValue, buckets }) => {
+ const features = [{ value: minValue }, { value: maxValue }];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: String(buckets) },
+ features,
+ accessor,
+ );
+
+ const firstBp = parseFloat(breakPoints[0]);
+ const lastBp = parseFloat(breakPoints[breakPoints.length - 1]);
+
+ // First breakpoint should be floored (always <= minValue)
+ expect(firstBp).toBeLessThanOrEqual(minValue);
+
+ // Last breakpoint should be ceiled (always >= maxValue)
+ expect(lastBp).toBeGreaterThanOrEqual(maxValue);
+
+ // All values should be within range
+ expect(minValue).toBeGreaterThanOrEqual(firstBp);
+ expect(maxValue).toBeLessThanOrEqual(lastBp);
+ });
+ });
+
+ it('prevents minimum value exclusion edge case', () => {
+ // Specific edge case test for minimum value exclusion
+ // Tests the exact scenario where rounding would exclude the min value
+
+ const features = [
+ { value: 3.14 }, // This would round to 3 at precision 0
+ { value: 50 },
+ { value: 100 },
+ ];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '5' },
+ features,
+ accessor,
+ );
+
+ const firstBp = parseFloat(breakPoints[0]);
+
+ // The first breakpoint must be <= 3.14 (floor behavior)
+ expect(firstBp).toBeLessThanOrEqual(3.14);
+
+ // Verify that 3.14 is not excluded
+ expect(3.14).toBeGreaterThanOrEqual(firstBp);
+
+ // The first breakpoint should be a clean floor value
+ expect(breakPoints[0]).toMatch(/^3(\.0+)?$/);
+ });
+
+ it('prevents maximum value exclusion edge case', () => {
+ // Specific edge case test for maximum value exclusion
+ // Tests the exact scenario where rounding would exclude the max value
+
+ const features = [
+ { value: 0 },
+ { value: 20 },
+ { value: 38.7 }, // Original bug case
+ ];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '5' },
+ features,
+ accessor,
+ );
+
+ const lastBp = parseFloat(breakPoints[breakPoints.length - 1]);
+
+ // The last breakpoint must be >= 38.7 (ceil behavior)
+ expect(lastBp).toBeGreaterThanOrEqual(38.7);
+
+ // Verify that 38.7 is not excluded
+ expect(38.7).toBeLessThanOrEqual(lastBp);
+
+ // The last breakpoint should be a clean ceil value
+ expect(breakPoints[breakPoints.length - 1]).toMatch(/^39(\.0+)?$/);
+ });
+ });
+
+ describe('custom breakpoints', () => {
+ it('uses custom breakpoints when provided', () => {
+ const features = [{ value: 5 }, { value: 15 }, { value: 25 }];
+ const customBreakPoints = ['0', '10', '20', '30', '40'];
+
+ const breakPoints = getBreakPoints(
+ { break_points: customBreakPoints, num_buckets: '' },
+ features,
+ accessor,
+ );
+
+ expect(breakPoints).toEqual(['0', '10', '20', '30', '40']);
+ });
+
+ it('sorts custom breakpoints in ascending order', () => {
+ const features = [{ value: 5 }];
+ const customBreakPoints = ['30', '10', '0', '20'];
+
+ const breakPoints = getBreakPoints(
+ { break_points: customBreakPoints, num_buckets: '' },
+ features,
+ accessor,
+ );
+
+ expect(breakPoints).toEqual(['0', '10', '20', '30']);
+ });
+
+ it('ignores num_buckets when custom breakpoints are provided', () => {
+ const features = [{ value: 5 }];
+ const customBreakPoints = ['0', '50', '100'];
+
+ const breakPoints = getBreakPoints(
+ { break_points: customBreakPoints, num_buckets: '10' }, // num_buckets
should be ignored
+ features,
+ accessor,
+ );
+
+ expect(breakPoints).toEqual(['0', '50', '100']);
+ expect(breakPoints).toHaveLength(3); // not 11
+ });
+ });
+
+ describe('edge cases and error handling', () => {
+ it('returns empty array when features are undefined', () => {
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '5' },
+ undefined as any,
+ accessor,
+ );
+
+ expect(breakPoints).toEqual([]);
+ });
+
+ it('returns empty array when features is null', () => {
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '5' },
+ null as any,
+ accessor,
+ );
+
+ expect(breakPoints).toEqual([]);
+ });
+
+ it('returns empty array when all values are undefined', () => {
+ const features = [
+ { value: undefined },
+ { value: undefined },
+ { value: undefined },
+ ];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '5' },
+ features,
+ accessor,
+ );
+
+ expect(breakPoints).toEqual([]);
+ });
+
+ it('handles empty features array', () => {
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '5' },
+ [],
+ accessor,
+ );
+
+ expect(breakPoints).toEqual([]);
+ });
+
+ it('handles string values that can be parsed as numbers', () => {
+ const features = [
+ { value: '10.5' },
+ { value: '20.3' },
+ { value: '30.7' },
+ ];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '3' },
+ features,
+ (d: any) => (typeof d.value === 'string' ? parseFloat(d.value) :
d.value),
+ );
+
+ const firstBp = parseFloat(breakPoints[0]);
+ const lastBp = parseFloat(breakPoints[breakPoints.length - 1]);
+
+ expect(firstBp).toBeLessThanOrEqual(10.5);
+ expect(lastBp).toBeGreaterThanOrEqual(30.7);
+ });
+
+ it('uses default number of buckets when not specified', () => {
+ const features = [{ value: 0 }, { value: 100 }];
+
+ const breakPoints = getBreakPoints(
+ { break_points: [], num_buckets: '' },
+ features,
+ accessor,
+ );
+
+ // Should use DEFAULT_NUM_BUCKETS (10)
+ expect(breakPoints).toHaveLength(11); // 10 buckets = 11 breakpoints
+ });
+
+ it('handles Infinity and -Infinity values', () => {
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Infinity values cause function failure</b></div>
<div id="fix">
The test case for handling Infinity and -Infinity values will cause the
`getBreakPoints` function to fail. When `minValue` is -Infinity and `maxValue`
is Infinity, the delta calculation `(maxValue - minValue) / numBuckets` results
in NaN, causing subsequent precision calculations and breakpoint generation to
fail. The function needs to handle infinite values by returning an empty array
or the test should expect this behavior.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
- if (minValue === undefined || maxValue === undefined) {
- return [];
- }
+ if (minValue === undefined || maxValue === undefined) {
+ return [];
+ }
+ if (!isFinite(minValue) || !isFinite(maxValue)) {
+ return [];
+ }
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/35033#issuecomment-3261658269>#3ebed4</a></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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
