Skip to content

Data Validator for Custom UN Imports#2094

Open
HarishC727 wants to merge 1 commit into
datacommonsorg:masterfrom
HarishC727:un_data_validator
Open

Data Validator for Custom UN Imports#2094
HarishC727 wants to merge 1 commit into
datacommonsorg:masterfrom
HarishC727:un_data_validator

Conversation

@HarishC727

Copy link
Copy Markdown
Contributor

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces an automated UN Dataset Validation Suite, including a shell wrapper, rule documentation, test data, and Python scripts to validate datasets against Data Commons Project schema mapping rules. The code review identified several critical issues and bugs: a 0-based vs 1-based index mismatch in the completeness check (Rule 16), fragile line-by-line MCF parsing that breaks continuation lines (Rule 15) or misses out-of-order properties (Rule 17), a missing implementation of the alternateName check (Rule 12), and a contradiction in Rule 13 where missing names cause failures instead of warnings. Additionally, the isolated test runner lacks coverage for rules 15–17, the regex in Rule 10 poses a ReDoS risk, file paths are fragilely hardcoded in Rule 3, and CSV readers should use utf-8-sig to safely handle Byte Order Marks.

Comment on lines +105 to +108
col_idx = df_in.columns.get_loc(col_name) # 0-based column index

# Construct expected coordinate
coord = f"{filename}:{csv_row_num}:{col_idx}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The #input coordinate in the output CSV (e.g., SDG_q1-2026_OBS_AG_FLS_PCT.csv:2:4) uses a 1-based column index. However, df_in.columns.get_loc(col_name) returns a 0-based index (e.g., 3 for the 4th column). This mismatch will cause the completeness validation to fail for every single row. Please adjust col_idx to be 1-based.

Suggested change
col_idx = df_in.columns.get_loc(col_name) # 0-based column index
# Construct expected coordinate
coord = f"{filename}:{csv_row_num}:{col_idx}"
col_idx = df_in.columns.get_loc(col_name) + 1 # 1-based column index
# Construct expected coordinate
coord = f"{filename}:{csv_row_num}:{col_idx}"

Comment on lines +60 to +73
for line_num, line in enumerate(f, 1):
line = line.strip()
if not line or line.startswith('#'):
continue

if line.startswith('Node:'):
current_node = line.split('Node:')[1].strip()
seen_properties = set()
continue

if current_node and ':' in line:
# Split on the first colon to get the property key
parts = line.split(':', 1)
prop_key = parts[0].strip()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Stripping leading whitespace from lines before parsing them breaks MCF continuation lines. In MCF syntax, lines starting with spaces or tabs are continuations of the previous property's value. If a continuation line contains a colon (e.g., inside a description), stripping the leading whitespace will cause the parser to treat it as a new property, leading to false positive duplicate property errors. Please skip continuation lines instead.

                    for line_num, line in enumerate(f, 1):
                        clean_line = line.strip()
                        if not clean_line or clean_line.startswith('#'):
                            continue
                            
                        if clean_line.startswith('Node:'):
                            current_node = clean_line.split('Node:')[1].strip()
                            seen_properties = set()
                            continue
                            
                        # Skip continuation lines (which start with space/tab in MCF)
                        if line.startswith(' ') or line.startswith('\t'):
                            continue
                            
                        if current_node and ':' in clean_line:
                            # Split on the first colon to get the property key
                            parts = clean_line.split(':', 1)
                            prop_key = parts[0].strip()

Comment on lines +38 to +40
class Rule12Validator(BaseRuleValidator):
def validate(self):
self.setup_logging("Rule 12 (Names Validation)")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The implementation of Rule12Validator completely omits checking alternateName against the DSD file (Rule 12.1), despite claiming to validate Rule 12. It currently only checks name against PV maps (Rule 12.2). Please implement the missing alternateName validation logic.

Comment on lines +58 to +63
name_match = re.search(r'name:\s*"([^"]+)"', node)
if not name_match:
# Depending on pipeline, some generic statvars might not have generated names, but we should flag it
# But wait, some might just be properties. We already filtered by typeOf: StatisticalVariable
errors.append(f"StatVar in {filename} is missing a 'name' property.")
continue

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There is a contradiction between the implementation and the design documentation for Rule 13. The documentation (rule_13_1_statvar_name_template.md) states that this check should be skipped (passed with warnings) because the pipeline does not reliably generate the name property. However, the current implementation actually fails the validation if name is missing. Please update the code to log a warning instead of failing.

Suggested change
name_match = re.search(r'name:\s*"([^"]+)"', node)
if not name_match:
# Depending on pipeline, some generic statvars might not have generated names, but we should flag it
# But wait, some might just be properties. We already filtered by typeOf: StatisticalVariable
errors.append(f"StatVar in {filename} is missing a 'name' property.")
continue
name_match = re.search(r'name:\s*"([^"]+)"', node)
if not name_match:
# Log a warning instead of failing, as per the skipped execution design
self.write_log(f"WARNING: StatVar in {filename} is missing a 'name' property.")
continue

Comment on lines +60 to +64
validators = [
Rule1Validator, Rule2Validator, Rule3Validator, Rule4Validator,
Rule5And7Validator, Rule6Validator, Rule8Validator, Rule9Validator, Rule10Validator, Rule11Validator,
Rule12Validator, Rule13Validator, Rule14Validator
]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The isolated test runner is missing the newly added rules 15, 16, and 17. Please import and add them to the validators list to ensure they are covered by the isolated test suite.

Suggested change
validators = [
Rule1Validator, Rule2Validator, Rule3Validator, Rule4Validator,
Rule5And7Validator, Rule6Validator, Rule8Validator, Rule9Validator, Rule10Validator, Rule11Validator,
Rule12Validator, Rule13Validator, Rule14Validator
]
validators = [
Rule1Validator, Rule2Validator, Rule3Validator, Rule4Validator,
Rule5And7Validator, Rule6Validator, Rule8Validator, Rule9Validator, Rule10Validator, Rule11Validator,
Rule12Validator, Rule13Validator, Rule14Validator, Rule15Validator, Rule16Validator, Rule17Validator
]

Comment on lines +67 to +74
if stripped_line.startswith("typeOf: "):
current_node_type = stripped_line[8:].strip()
continue

if stripped_line.startswith("name: "):
# Skip StatVarGroup names as they are group titles and naturally contain classification codes
if current_node_type and "StatVarGroup" in current_node_type:
continue

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Parsing typeOf line-by-line is fragile because MCF properties can appear in any order within a node block. If name appears before typeOf in the MCF file, current_node_type will be None when name is validated, causing the StatVarGroup exclusion check to be bypassed and leading to false positive failures on group titles. Consider parsing the entire node block first before validating its properties.

Comment on lines +132 to +133
# Global geography names
geo_names_path = os.path.join(os.path.dirname(self.dataset_dir), "un_geography.csv")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The path to un_geography.csv is hardcoded relative to self.dataset_dir. This is fragile and can lead to brittle code if the repository is cloned into a different location or the path structure changes. Instead of hardcoding paths that rely on specific directory structures, use more robust methods to locate the repository root, such as searching for marker files (e.g., .git or WORKSPACE), or allow the path to be passed as an argument.

References
  1. Avoid hardcoding paths that rely on specific directory structures (e.g., '/data/') as this can lead to brittle code that breaks if the repository is cloned into a different location or the path structure changes. Instead, use more robust methods to locate the repository root, such as searching for marker files (e.g., .git or WORKSPACE).

Comment on lines +126 to +127
block_pattern = rf"Node: dcid:[^\n]+(?:\n(?!\n).*)*unConcept: \"{attr_upper}\"(?:\n(?!\n).*)*"
match_block = re.search(block_pattern, schema_content)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The regular expression block_pattern uses nested quantifiers (?:\n(?!\n).*)* which can lead to catastrophic backtracking (ReDoS) on large MCF files. A much simpler and safer approach is to split the MCF content by double newlines \n\n to get each node block, and then check for the presence of the unConcept string.

Comment on lines +53 to +54
with open(filepath, 'r', encoding='utf-8') as f:
reader = csv.DictReader(f)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Opening CSV files with encoding='utf-8' can cause parsing failures if the file contains a Byte Order Mark (BOM), as the BOM will be prepended to the first column name (e.g., \ufeffUnConcept), causing column matching to fail. Using encoding='utf-8-sig' is much safer and more robust for all CSV readers in this project.

Suggested change
with open(filepath, 'r', encoding='utf-8') as f:
reader = csv.DictReader(f)
with open(filepath, 'r', encoding='utf-8-sig') as f:
reader = csv.DictReader(f)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant