msyavuz commented on code in PR #43453:
URL: https://github.com/apache/superset/pull/43453#discussion_r3882181103
##########
superset-frontend/packages/superset-ui-core/src/number-format/index.ts:
##########
@@ -19,6 +19,8 @@
export { default as NumberFormats } from './NumberFormats';
export { default as NumberFormatter, PREVIEW_VALUE } from './NumberFormatter';
+export { formatSpecifier } from 'd3-format';
+export type { FormatLocaleDefinition } from 'd3-format';
Review Comment:
`@types/d3-format` is a devDependency of core and `d3-format` ships no types
of its own, so this public type re-export won't resolve for installed consumers
— pre-existing (`lib/number-format/NumberFormatterRegistry.d.ts` already leaks
it), but worth promoting to `dependencies` while you're here?
##########
superset-frontend/package.json:
##########
@@ -146,7 +146,6 @@
"@superset-ui/plugin-chart-world-map":
"file:./plugins/plugin-chart-world-map",
"@superset-ui/preset-chart-deckgl": "file:./plugins/preset-chart-deckgl",
"@superset-ui/switchboard": "file:./packages/superset-ui-switchboard",
- "@types/d3-format": "^3.0.1",
Review Comment:
`package-lock.json` still records this under the root's dependencies —
unlike the missing entry that broke the build earlier, `npm ci` tolerates a
stale one, so it's hygiene rather than a failure.
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -827,6 +828,64 @@ function EditorsSelector({
const ResultTable =
extensionsRegistry.get('sqleditor.extension.resultTable') ?? FilterableTable;
+// D3's '%' and 'p' types both multiply by 100; parsed via d3-format's own
+// grammar so garbage like "foo%" is rejected rather than matched by suffix.
+export const isPercentD3Format = (d3format?: string): boolean => {
+ const trimmed = d3format?.trim();
+ if (!trimmed) {
+ return false;
+ }
+ try {
+ const { type } = formatSpecifier(trimmed);
+ return type === '%' || type === 'p';
+ } catch {
+ return false;
+ }
+};
+
+// Matches the outermost COUNT(...) call's parens by depth, so a ratio like
+// `COUNT(*) / COUNT(*)` isn't misclassified but a nested call like
+// `COUNT(DISTINCT COALESCE(a, b))` is still recognized.
+export const isCountExpression = (expression?: string): boolean => {
+ const trimmed = expression?.trim();
+ if (!trimmed || !/^count\s*\(/i.test(trimmed) || !trimmed.endsWith(')')) {
+ return false;
+ }
+ let depth = 0;
+ for (let i = trimmed.indexOf('('); i < trimmed.length; i += 1) {
Review Comment:
The depth scan counts parens inside string literals, so `COUNT(CASE WHEN x =
'(' THEN 1 END)` gets no warning — fine as a false negative?
--
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]
