tnightengale / dbt-activity-schema

A dbt-Core package for generating models from an activity stream.
GNU General Public License v3.0
38 stars 5 forks source link

Enable parsing of feature json columns #34

Closed bcodell closed 1 year ago

bcodell commented 1 year ago

This PR:

bcodell commented 1 year ago

@tnightengale looking for the following feedback:

tnightengale commented 1 year ago

@tnightengale looking for the following feedback:

  • Was not being able to specify a feature_json key as an included column a bug or did this functionality intentionally not exist? It seems like you intended to implement it since you had the json_unpack_key helper function already in place (asking for semantic versioning purposes)
  • Does the abstraction I put in place make sense, or are there changes you'd like to see?
  • Does this assumption (baked into the code make sense: that if a specified included column isn't identified as one of the standard columns (or project-specific aliases) from the Activity Schema spec, it is assumed to be contained in the feature_json?
  • Are the semantic changes I made to example__activity_stream (removing feature_json from array, snake-casing json keys) reasonable?
  1. It was not an intended feature, since folks could just add another CTE below and unpack the columns there. But this is a fine convenience feature.
  2. The abstraction being the parse_column function? Yes I think it makes sense.
  3. The assumption about an included column not in the default columns, being in feature json makes sense.
  4. Lgtm!
bcodell commented 1 year ago

Thanks @tnightengale! I'm going to merge this then open a PR to bump the version to 0.4.0. This feature should provide a workaround to #26 and #33 until those bugs are explicitly resolved.