-
Notifications
You must be signed in to change notification settings - Fork 328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Drive data quality metrics from enso code #11638
Changes from all commits
4c2af9b
44ddaf7
1d8513c
a25934d
1e4d427
7d43b50
529914f
ee75297
01c5a04
a0d2099
5cd54e8
e08c878
ee1a7de
29ba984
370f3d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,8 +13,7 @@ export class TableVisualisationTooltip implements ITooltipComp { | |
*/ | ||
init( | ||
params: ITooltipParams & { | ||
numberOfNothing: number | ||
numberOfWhitespace: number | ||
dataQualityMetrics: Record<string, number>[] | ||
total: number | ||
showDataQuality: boolean | ||
}, | ||
|
@@ -32,7 +31,6 @@ export class TableVisualisationTooltip implements ITooltipComp { | |
}) | ||
|
||
const getPercentage = (value: number) => ((value / params.total) * 100).toFixed(2) | ||
const getDisplay = (value: number) => (value > 0 ? 'block' : 'none') | ||
const createIndicator = (value: number) => { | ||
const color = | ||
value < 33 ? 'green' | ||
|
@@ -41,20 +39,29 @@ export class TableVisualisationTooltip implements ITooltipComp { | |
return `<div style="display: inline-block; width: 10px; height: 10px; border-radius: 50%; background-color: ${color}; margin-left: 5px;"></div>` | ||
} | ||
|
||
const dataQualityTemplate = ` | ||
<div style="display: ${getDisplay(params.numberOfNothing)};"> | ||
Nulls/Nothing: ${getPercentage(params.numberOfNothing)}% ${createIndicator(+getPercentage(params.numberOfWhitespace))} | ||
</div> | ||
<div style="display: ${getDisplay(params.numberOfWhitespace)};"> | ||
Trailing/Leading Whitespace: ${getPercentage(params.numberOfWhitespace)}% ${createIndicator(+getPercentage(params.numberOfWhitespace))} | ||
</div> | ||
` | ||
const getDataQualityTemplate = () => { | ||
let template = '' | ||
params.dataQualityMetrics.forEach((obj) => { | ||
const key = Object.keys(obj)[0] | ||
const value = key ? obj[key] : null | ||
if (key && value) { | ||
const metricTemplate = `<div> | ||
${key}: ${getPercentage(value)}% ${createIndicator(+getPercentage(value))} | ||
</div>` | ||
template = template + metricTemplate | ||
} else { | ||
console.warn( | ||
'Data quality metric is missing a valid key-value pair. Ensure each object in data_quality_pairs contains a single valid key with a numeric value.', | ||
) | ||
} | ||
}) | ||
return template | ||
} | ||
|
||
this.eGui.innerHTML = ` | ||
<div><b>Column value type:</b> ${params.value}</div> | ||
<div style="display: ${params.showDataQuality ? 'block' : 'none'};""> | ||
<b>Data Quality Indicators</b> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed intentionally? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes - we didn't see the point having the header in the tooltip. |
||
${dataQualityTemplate} | ||
${getDataQualityTemplate()} | ||
</div> | ||
` | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,12 +81,12 @@ interface UnknownTable { | |
get_child_node_action: string | ||
get_child_node_link_name: string | ||
link_value_type: string | ||
data_quality_pairs?: DataQualityPairs | ||
data_quality_metrics?: DataQualityMetric[] | ||
} | ||
|
||
interface DataQualityPairs { | ||
number_of_nothing: number[] | ||
number_of_whitespace: number[] | ||
type DataQualityMetric = { | ||
name: string | ||
percentage_value: number[] | ||
} | ||
|
||
export type TextFormatOptions = 'full' | 'partial' | 'off' | ||
|
@@ -356,23 +356,15 @@ function toField( | |
const displayValue = valueType ? valueType.display_text : null | ||
const icon = valueType ? getValueTypeIcon(valueType.constructor) : null | ||
|
||
const dataQuality = | ||
typeof props.data === 'object' && 'data_quality_pairs' in props.data ? | ||
props.data.data_quality_pairs | ||
// eslint-disable-next-line camelcase | ||
: { number_of_nothing: [], number_of_whitespace: [] } | ||
|
||
const nothingIsNonZero = | ||
index != null && dataQuality?.number_of_nothing ? | ||
(dataQuality.number_of_nothing[index] ?? 0) > 0 | ||
: false | ||
|
||
const whitespaceIsNonZero = | ||
index != null && dataQuality?.number_of_nothing ? | ||
(dataQuality.number_of_whitespace[index] ?? 0) > 0 | ||
: false | ||
const dataQualityMetrics = | ||
typeof props.data === 'object' && 'data_quality_metrics' in props.data ? | ||
props.data.data_quality_metrics.map((metric: DataQualityMetric) => { | ||
return { [metric.name]: metric.percentage_value[index!] ?? 0 } | ||
}) | ||
: [] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Log an error in this case as well? (Assuming it is a malformed data error...) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not an error just no metrics sent. |
||
|
||
const showDataQuality = nothingIsNonZero || whitespaceIsNonZero | ||
const showDataQuality = | ||
dataQualityMetrics.filter((obj) => (Object.values(obj)[0] as number) > 0).length > 0 | ||
|
||
const getSvgTemplate = (icon: string) => | ||
`<svg viewBox="0 0 16 16" width="16" height="16"> <use xlink:href="${icons}#${icon}"/> </svg>` | ||
|
@@ -401,8 +393,7 @@ function toField( | |
tooltipComponent: TableVisualisationTooltip, | ||
headerTooltip: displayValue ? displayValue : '', | ||
tooltipComponentParams: { | ||
numberOfNothing: index != null ? dataQuality.number_of_nothing[index] : null, | ||
numberOfWhitespace: index != null ? dataQuality.number_of_whitespace[index] : null, | ||
dataQualityMetrics, | ||
total: typeof props.data === 'object' ? props.data.all_rows_count : 0, | ||
showDataQuality, | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value
truthiness check here seems to makegetDisplay
redundant.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get an
else
branch that does someconsole.log
telling us that a metric format is malformed?This was one of the problems with debugging visualizations - there are similar situations where it just works or just doesn't work but doesn't tell us why. Let's try to log these bad conditions so that we can actually figure out what is wrong. IMHO every
if
that checks a required condition should have an else branch logging that it is violated.