Skip to content

Cdspt 15587 align functionality of totals methods for all extractor subclasses#18

Merged
TiffanyCheng27 merged 8 commits into
mainfrom
CDSPT-15587-align-functionality-of-totals-methods-for-all-extractor-subclasses
Jun 24, 2026
Merged

Cdspt 15587 align functionality of totals methods for all extractor subclasses#18
TiffanyCheng27 merged 8 commits into
mainfrom
CDSPT-15587-align-functionality-of-totals-methods-for-all-extractor-subclasses

Conversation

@scottbw

@scottbw scottbw commented Jun 19, 2026

Copy link
Copy Markdown
Member

📌 Summary

This change ensures that each Extractor subclass contains:

get_row_count()
get_total(measure:str)

Calling code should now call either:

  • get_row_count() if you want a row count
  • get_total(measure) if you want to sum by a specific measure
  • calling get_total() is discouraged.

The behaviour is consistent across implementations:

  • get_row_count() returns the row count
  • get_total(measure) returns SUM of measure if measure is not None and that measure exists in dataset.measures
  • get_total(measure) raises ValueError when measure is not None and the measure does not exist in dataset.measures
  • get_total(measure) returns SUM of the first measure in dataset.measures if measure is None and there are measures in dataset.measures
  • get_total(measure) returns the row count if measure is None and there are no measures in dataset.measures

Notes:

  1. this is slightly different from the previous behaviour, where if a measure was specified that did not exist, the row_count was returned.
  2. 'Find a default measure' logic was previously also in SubsetQueryBuilder. I've removed this - the only place default measures are substituted for None is now in the Extractor layer.

🧪 What Changed?

  • New feature
  • Bug fix
  • Refactor
  • Documentation only
  • CI/CD or tooling
  • Other (please explain):

✅ Checklist

Code Quality

  • [x ] Code is clean, commented, and follows our conventions
  • [x ] No hardcoded values or secrets
  • [x ] Reusable and modular where possible

Testing

  • I have tested this change locally
  • Relevant tests have been added/updated
  • All existing tests still pass

Documentation

  • Code comments or inline explanations added where needed
  • README / documentation updated (if applicable)
  • Any environment variables added are documented

Security & Deployment

  • No sensitive data in code or logs
  • No breaking changes introduced
  • Deploy scripts / jobs updated (if needed)

Note that there is a change in behaviour for where a non-existent measure is specified for a total.

⚙️ Actions Required After Pulling

  • Yes.
  • No.

If you checked Yes, please provide instructions below:


🧠 Context or Screenshots


🚨 Risk & Impact

  • No user-facing changes
  • Minimal risk — small changes isolated to specific areas
  • Medium risk — changes affect multiple areas
  • High risk — could impact critical paths or user flows

Notes for reviewers (optional):

scottbw added 5 commits June 19, 2026 11:29
… get_row_count are the same across Extracto classes
…not in dataset.measures raises an error rather than silently giving you the row count.
…t a request for a query with measure None we just return Count(*). Substituting measures happens in the Extractor layer now.
@scottbw scottbw requested a review from TiffanyCheng27 June 22, 2026 06:50
@TiffanyCheng27

Copy link
Copy Markdown
Contributor
  • Add a unit test for the Athena get_row_count() function.
  • The current unit tests cover HyperFile and DataExtractor. Add unit tests for StreamingDataExtractor to cover get_row_count() and get_total().
  • (Optional) Additionally, since everything works fine for now, this can be change in the future. It might be worth considering making the get_total() function more consistent across extractors. Currently some extractors delegate internally to __get_total__() while others don't use that sub-layer at all. And there are duplication in StreamingDataExtractor and AthenaDataExtractor. We can think about standardising the pattern so all extractors follow the same structure.

@scottbw

scottbw commented Jun 23, 2026

Copy link
Copy Markdown
Member Author

I've added the missing unit tests and refactored so they all call get_total() internal metho

@TiffanyCheng27 TiffanyCheng27 merged commit a29fc2e into main Jun 24, 2026
3 checks passed
@TiffanyCheng27 TiffanyCheng27 deleted the CDSPT-15587-align-functionality-of-totals-methods-for-all-extractor-subclasses branch June 24, 2026 09:56
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.

2 participants