Move null type to first position in type lists #9
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Beautiful ! this is great to see, thank you @marknotfound ! |
|
@hack-c can you pull this branch and confirm this solves the issue with target-bq before I give it the stamp of approval? |
|
@dluftspring This PR may not be necessary since the bug it's solving for in the bigquery target is actually in what looks like an unmaintained repo. @hack-c I emailed you about this already but posting here for Dan as well. The bug is in https://github.com/RealSelf/target-bigquery/ but the repo listed on MeltanoHub is https://github.com/adswerve/target-bigquery which is a more recent fork of the other. |
|
Very helpful thanks @marknotfound ! Going to add a comment to the issue. |
|
@hack-c I'm happy to merge this if it will unblock you. Is the issue still persisting or was forking the adswerve repo enough to solve the problem? |
dluftspring
approved these changes
Feb 23, 2022
|
Hi @dluftspring - as it turns out, we were using the adswerve version all
along with Meltano. Sorry for the late response. The link on the pypi was
broken and pointed to the old repo.
…On Wed, Feb 23, 2022 at 10:05 AM dluftspring ***@***.***> wrote:
***@***.**** approved this pull request.
👍
—
Reply to this email directly, view it on GitHub
<#9 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAWPRRB4MUQWTYMO3I7QOGTU4TZTZANCNFSM5OV2TTNQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
All good! Can you close the issue then? |
|
I meant that the adswerve variant is the one that is giving the error. |
|
I can't make much out of this stack trace but maybe you can? |
|
@hack-c im trying to get to the bottom of this but it looks like a bug with the target. It's trying to parse a message of type |
|
Thanks so much for your help on this @dluftspring . In order to test it, would I just update the git URL in the |
|
I reinstalled from @marknotfound 's fork and if failed with the same error :/ sorry @marknotfound |
|
here's my (baton-elt) ➜ baton-elt cat meltano.yml
version: 1
send_anonymous_usage_stats: true
project_id: c417c22a-fb9a-4e0d-8f28-69d2cb1d2671
plugins:
extractors:
- name: tap-hellobaton
namespace: tap_hellobaton
pip_url: git+https://github.com/marknotfound/tap-hellobaton.git
executable: tap-hellobaton
capabilities:
- catalog
- state
- discover
settings:
- name: company
kind: string
- name: api_key
kind: password
- name: user_agent
kind: string
config:
company: mantl
user_agent: Singer Tap for hellobaton
load_schema: baton
loaders:
- name: target-bigquery
variant: adswerve
pip_url: git+https://github.com/adswerve/[email protected]
config:
project_id: mantl-edw-datalake-production
dataset_id: baton
- name: target-jsonl
variant: andyh1203
pip_url: target-jsonl |
|
@hack-c are you using a catalogue when you sync records? Sorry this is taking a while to track down. I think the issue is with how the target handles nested JSON. So in cases where the schema declares a type as |
|
@marknotfound in the |
|
The `meta` field in the Activity API will be an object (dict if parsed with
python as you've pointed out) with arbitrary data related to the event. For
some event types this will be empty: (`{}`). The docs likely list it as a
string because behind the scenes it is implemented as a JSON field in
MySQL. I will see if I can manually override that somehow in our automated
docs output.
…On Thu, Feb 24, 2022 at 4:47 PM dluftspring ***@***.***> wrote:
@marknotfound <https://github.com/marknotfound> in the activity endpoint
what type is meta supposed to be? It consistently sends me an empty
dictionary but on the docs it's listed as a string. I think the issue is
that target-bigquery can't handle cases where a field is listed as an
object but does not have specified property types in the json blob or
nested structure. The fix to this may be as simple as casting the field to
a string in transit or updating the type definition in the json schema
—
Reply to this email directly, view it on GitHub
<#9 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALADCVGDAPAEG6IJ7QFVADU42RO5ANCNFSM5OV2TTNQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
|
|
Okay so after some investigation I think the issue is definitely with the
UPDATE: You can also try pulling from the branch on PR #10 |
|
@dluftspring @marknotfound - thank you guys so much for this !!! going to
try these now.
…On Fri, Feb 25, 2022 at 9:22 AM dluftspring ***@***.***> wrote:
Okay so after some investigation I think the issue is definitely with the
meta field within the activity endpoint. @hack-c
<https://github.com/hack-c> here are some things to try (I don't have
easy access to a BQ test environment or i'd do these myself).
1. Use the force fields
<https://github.com/adswerve/target-bigquery#example> option to cast
meta to a string
2. Use a catalogue <https://hub.meltano.com/singer/spec#catalog-files>
to unselect the meta field altogether
3. I'll make a PR that casts the meta field to a string type (even
though it's a jsonb) in transit and then any downstream work will have to
be done in the warehouse.
—
Reply to this email directly, view it on GitHub
<#9 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAWPRRH7HYLVZOGWYN4ZOMLU46GBVANCNFSM5OV2TTNQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
OMG ! @dluftspring the first option worked ! forcing the |
|
🎸 woohoo! any chance you can see if #10 solves it as well? I'd rather have something that works for everyone but if not i'll close the PRs and update the README with a call out for bigquery users |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description & motivation
I believe this should fix #8 which to me is definitely an issue with bigquery's target (see RealSelf/target-bigquery#10 linked in above issue), but one we can fix here as well.
This is a...
Screenshots
Open questions
How to test