Repository navigation
Commit d5216e6
feat: add GroupColumn support for Decimal32/Decimal64 (#25470)
## Which issue does this PR close?
- Part of #24268.
## Rationale for this change
`Decimal32` and `Decimal64` group-by keys currently force the whole
grouping onto
the row-encoded `GroupValuesRows` fallback.
`group_column_supported_type` is an all-or-nothing gate: if any one
column in a
multi-column key is missing from it, `new_group_values` drops the
*entire* key to
`GroupValuesRows`, so a `(Utf8, Decimal32)` key loses the column-wise
path for
its string column too:
```
(Utf8, Decimal32 ) -> GroupValuesRows
(Utf8, Decimal64 ) -> GroupValuesRows
(Utf8, Decimal128) -> GroupValuesColumn
```
DataFusion has supported these two widths since #17501, and #23849 added
`Decimal256` to the allow-list, but the two narrow widths were never
backfilled.
Both implement `ArrowPrimitiveType`, so they reuse the existing
`PrimitiveGroupValueBuilder` — no new builder is required, exactly like
`Decimal128` and `Decimal256`.
The gap is also internally inconsistent today: `Struct("a": Decimal32)`
*is*
already supported, because nested types reach the generic
`RowsGroupColumn`
fallback added in #23523. Only the bare top-level type is rejected.
## What changes are included in this PR?
- Support `Decimal32` / `Decimal64` in `group_column_supported_type` and
`make_group_column`.
- `Dictionary(K, Decimal32 | Decimal64)` starts working as a side effect
of the
existing dictionary recursion in both functions — no extra code.
- Extend the `group_column_supported_type_matches_make_group_column`
biconditional test with the two scalar types and the two
dictionary-wrapped
variants.
- Add `test_group_values_column_narrow_decimals`, which drives both
widths
through one body rather than only the first, asserting
`supported_schema`
routing, dedup including nulls, and that precision/scale survive `emit`.
- Add a `bench_narrow_decimals` benchmark mirroring `bench_decimal256`.
## Are these changes tested?
Yes.
- The consistency test plus the new
`test_group_values_column_narrow_decimals`
round-trip test in `multi_group_by/mod.rs`. The round-trip test includes
a
value at each type's full width (`999_999_999` and
`999_999_999_999_999_999`) so a truncating storage type would fail it.
- Multi-column `Decimal32` / `Decimal64` GROUP BY with a NULL key in
`group_by.slt`. These widths are not reachable from a SQL `DECIMAL(p,
s)`
declaration — that maps to `Decimal128` or `Decimal256` by precision —
so the
keys are built with `arrow_cast`, and each case is paired with an
`arrow_typeof` assertion so it cannot silently degrade into more
`Decimal128`
coverage.
`cargo test -p datafusion-physical-plan --lib` (1921 passed),
`aggregate.slt group_by.slt dictionary.slt decimal.slt`, and
`dev/rust_lint.sh` are all clean.
## Are there any user-facing changes?
No. This only changes which `GroupValues` implementation is selected;
grouping
semantics and output types are unchanged.
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>1 parent 57b830b commit d5216e6
3 files changed
Lines changed: 255 additions & 10 deletions
File tree
- datafusion
- physical-plan
- benches
- src/aggregates/group_values/multi_group_by
- sqllogictest/test_files
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
29 | 29 | | |
30 | 30 | | |
31 | 31 | | |
32 | | - | |
33 | | - | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
34 | 35 | | |
35 | 36 | | |
36 | 37 | | |
| |||
1011 | 1012 | | |
1012 | 1013 | | |
1013 | 1014 | | |
| 1015 | + | |
| 1016 | + | |
| 1017 | + | |
| 1018 | + | |
| 1019 | + | |
| 1020 | + | |
| 1021 | + | |
| 1022 | + | |
| 1023 | + | |
| 1024 | + | |
| 1025 | + | |
| 1026 | + | |
| 1027 | + | |
| 1028 | + | |
| 1029 | + | |
| 1030 | + | |
| 1031 | + | |
| 1032 | + | |
| 1033 | + | |
| 1034 | + | |
| 1035 | + | |
| 1036 | + | |
| 1037 | + | |
| 1038 | + | |
| 1039 | + | |
| 1040 | + | |
| 1041 | + | |
| 1042 | + | |
| 1043 | + | |
| 1044 | + | |
| 1045 | + | |
| 1046 | + | |
| 1047 | + | |
| 1048 | + | |
| 1049 | + | |
| 1050 | + | |
| 1051 | + | |
| 1052 | + | |
| 1053 | + | |
| 1054 | + | |
| 1055 | + | |
| 1056 | + | |
| 1057 | + | |
| 1058 | + | |
| 1059 | + | |
| 1060 | + | |
| 1061 | + | |
| 1062 | + | |
| 1063 | + | |
| 1064 | + | |
| 1065 | + | |
| 1066 | + | |
| 1067 | + | |
| 1068 | + | |
| 1069 | + | |
| 1070 | + | |
| 1071 | + | |
| 1072 | + | |
| 1073 | + | |
| 1074 | + | |
| 1075 | + | |
| 1076 | + | |
| 1077 | + | |
| 1078 | + | |
| 1079 | + | |
| 1080 | + | |
| 1081 | + | |
| 1082 | + | |
| 1083 | + | |
| 1084 | + | |
| 1085 | + | |
| 1086 | + | |
| 1087 | + | |
| 1088 | + | |
| 1089 | + | |
| 1090 | + | |
| 1091 | + | |
| 1092 | + | |
| 1093 | + | |
| 1094 | + | |
| 1095 | + | |
| 1096 | + | |
| 1097 | + | |
| 1098 | + | |
| 1099 | + | |
| 1100 | + | |
| 1101 | + | |
| 1102 | + | |
| 1103 | + | |
| 1104 | + | |
| 1105 | + | |
| 1106 | + | |
| 1107 | + | |
| 1108 | + | |
| 1109 | + | |
| 1110 | + | |
| 1111 | + | |
| 1112 | + | |
| 1113 | + | |
| 1114 | + | |
| 1115 | + | |
| 1116 | + | |
| 1117 | + | |
| 1118 | + | |
| 1119 | + | |
1014 | 1120 | | |
1015 | 1121 | | |
1016 | 1122 | | |
| |||
1026 | 1132 | | |
1027 | 1133 | | |
1028 | 1134 | | |
| 1135 | + | |
1029 | 1136 | | |
1030 | 1137 | | |
Lines changed: 104 additions & 8 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
38 | 38 | | |
39 | 39 | | |
40 | 40 | | |
41 | | - | |
42 | | - | |
43 | | - | |
44 | | - | |
45 | | - | |
46 | | - | |
47 | | - | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
48 | 48 | | |
49 | 49 | | |
50 | 50 | | |
| |||
1036 | 1036 | | |
1037 | 1037 | | |
1038 | 1038 | | |
| 1039 | + | |
| 1040 | + | |
| 1041 | + | |
| 1042 | + | |
| 1043 | + | |
| 1044 | + | |
1039 | 1045 | | |
1040 | 1046 | | |
1041 | 1047 | | |
| |||
1333 | 1339 | | |
1334 | 1340 | | |
1335 | 1341 | | |
1336 | | - | |
| 1342 | + | |
| 1343 | + | |
| 1344 | + | |
1337 | 1345 | | |
1338 | 1346 | | |
1339 | 1347 | | |
| |||
1633 | 1641 | | |
1634 | 1642 | | |
1635 | 1643 | | |
| 1644 | + | |
| 1645 | + | |
1636 | 1646 | | |
1637 | 1647 | | |
1638 | 1648 | | |
| |||
1684 | 1694 | | |
1685 | 1695 | | |
1686 | 1696 | | |
| 1697 | + | |
| 1698 | + | |
| 1699 | + | |
| 1700 | + | |
| 1701 | + | |
| 1702 | + | |
| 1703 | + | |
| 1704 | + | |
| 1705 | + | |
| 1706 | + | |
1687 | 1707 | | |
1688 | 1708 | | |
1689 | 1709 | | |
| |||
1792 | 1812 | | |
1793 | 1813 | | |
1794 | 1814 | | |
| 1815 | + | |
| 1816 | + | |
| 1817 | + | |
| 1818 | + | |
| 1819 | + | |
| 1820 | + | |
| 1821 | + | |
| 1822 | + | |
| 1823 | + | |
| 1824 | + | |
| 1825 | + | |
| 1826 | + | |
| 1827 | + | |
| 1828 | + | |
| 1829 | + | |
| 1830 | + | |
| 1831 | + | |
| 1832 | + | |
| 1833 | + | |
| 1834 | + | |
| 1835 | + | |
| 1836 | + | |
| 1837 | + | |
| 1838 | + | |
| 1839 | + | |
| 1840 | + | |
| 1841 | + | |
| 1842 | + | |
| 1843 | + | |
| 1844 | + | |
| 1845 | + | |
| 1846 | + | |
| 1847 | + | |
| 1848 | + | |
| 1849 | + | |
| 1850 | + | |
| 1851 | + | |
| 1852 | + | |
| 1853 | + | |
| 1854 | + | |
| 1855 | + | |
| 1856 | + | |
| 1857 | + | |
| 1858 | + | |
| 1859 | + | |
| 1860 | + | |
| 1861 | + | |
| 1862 | + | |
| 1863 | + | |
| 1864 | + | |
| 1865 | + | |
| 1866 | + | |
| 1867 | + | |
| 1868 | + | |
| 1869 | + | |
| 1870 | + | |
| 1871 | + | |
| 1872 | + | |
| 1873 | + | |
| 1874 | + | |
| 1875 | + | |
| 1876 | + | |
| 1877 | + | |
| 1878 | + | |
| 1879 | + | |
| 1880 | + | |
| 1881 | + | |
| 1882 | + | |
| 1883 | + | |
| 1884 | + | |
| 1885 | + | |
| 1886 | + | |
| 1887 | + | |
| 1888 | + | |
| 1889 | + | |
| 1890 | + | |
1795 | 1891 | | |
1796 | 1892 | | |
1797 | 1893 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
5880 | 5880 | | |
5881 | 5881 | | |
5882 | 5882 | | |
| 5883 | + | |
| 5884 | + | |
| 5885 | + | |
| 5886 | + | |
| 5887 | + | |
| 5888 | + | |
| 5889 | + | |
| 5890 | + | |
| 5891 | + | |
| 5892 | + | |
| 5893 | + | |
| 5894 | + | |
| 5895 | + | |
| 5896 | + | |
| 5897 | + | |
| 5898 | + | |
| 5899 | + | |
| 5900 | + | |
| 5901 | + | |
| 5902 | + | |
| 5903 | + | |
| 5904 | + | |
| 5905 | + | |
| 5906 | + | |
| 5907 | + | |
| 5908 | + | |
| 5909 | + | |
| 5910 | + | |
| 5911 | + | |
| 5912 | + | |
| 5913 | + | |
| 5914 | + | |
| 5915 | + | |
| 5916 | + | |
| 5917 | + | |
| 5918 | + | |
| 5919 | + | |
| 5920 | + | |
| 5921 | + | |
| 5922 | + | |
| 5923 | + | |
| 5924 | + | |
0 commit comments