search

singer-io / tap-mongodb

GNU Affero General Public License v3.0
28 stars 38 forks source link

sum string projection as integer of bool #94

Closed rks0nax closed 6 months ago

rks0nax commented 1 year ago

Description of change

Fixes issue - #93 The change allows user to specify string projection, instead of just integer values

A sample projection that didn't work before and should be working after merging this PR {"name": "$personName"}

QA steps

Risks

Rollback steps

singer-bot commented 1 year ago

Hi @rks0nax, thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes.

singer-bot commented 1 year ago

You did it @rks0nax!

Thank you for signing the Singer Contribution License Agreement.

dmcquay commented 7 months ago

I need this fix badly. Looks like this has been ready to merge for a long time. What's the hold up? @rks0nax Thanks for reporting and contributing the fix! Was very happy to see this.

dmcquay commented 7 months ago

@cosimon can you provide an update on this?

dmcquay commented 7 months ago

In the Stitch troubleshooting documentation, their recommended course of action for column or table name too long loading errors is to omit the table from the source, rename at the source or switch to a different destination. https://www.stitchdata.com/docs/troubleshooting/destinations/destination-loading-error-reference#column-name-limit

This PR would allow us to instead use aliases in mongo projections to address the issue, which is MUCH less painful. Switching the destination of our entire data lake is obviously quite painful. Renaming at the source is painful because the source is an app tier DB used in a monolithic architecture -- hard to change. And simply omitting the data obviously only works well if I don't NEED the data in my data lake.

Just providing context on why this PR is valuable to me.

dmcquay commented 7 months ago

And please notice that the change is:

  1. Only one line
  2. Seems to me like disallowing string values here was an accident in the first place, not strategic. So this really just feels like a bug fix.

Seems like it is a no brainer to get this merged.