GL Work by romilqlik · Pull Request #7 · singer-io/tap-amplitude

romilqlik · 2025-06-11T13:38:08Z

Description of Changes

SAC-27712 -

updated init.py
Auto detect replication keys
Mark all valid fields and streams
Add date time format for time fields
updated incremental.py to handle filtering and bookmarks correctly

Manual QA Steps

Discovery: Running
Sync: Tested end to end, data fetched
Unit Tests: Removed
CI status: Removed (Will take care in the Bug)
Verified successful sync and state updates for sample test table.

Risks

Minor risk if replication key is missing or schema format doesn't match.

Rollback Steps

Revert the PR merge if needed.

singer-bot · 2025-06-11T13:38:10Z

Hi @romilqlik, 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.

…al sync

…rmental sync

RushiT0122 · 2025-06-17T12:44:35Z

-          'pytz==2018.4',
-          'singer-python==5.1.5',
-          'backoff==1.3.2',
+          'asn1crypto==1.5.1',


Please check if this is whitelisted dependancy.

Romil - This has been discussed with Rutuja. Kindly confirm if it's whitelisted.

I would like wait until dependency is approved since we may have to update the existing implementation if not approved.

vishalp-dev · 2025-06-17T14:09:20Z

    result = Schema(inclusion=inclusion)

-    if data_type == 'boolean':
+    if data_type in ['boolean']:


if data_type in ['boolean']
This seems un-necessary for checking with a single item in a list, is this faster in comparison to if data_type == 'boolean' ?

If we want to make this more clear and concise, this is how we can write it.

STRING_SCHEMA = {'type': ['null', 'string']} NUMBER_SCHEMA = {'type': ['null', 'number']} DATETIME_SCHEMA = {'type': ['null', 'string'], 'format': 'date-time'} SCHEMA_MAPPING = { 'boolean': {'type': ['null', 'boolean']}, 'number': NUMBER_SCHEMA, 'real': NUMBER_SCHEMA, 'float': NUMBER_SCHEMA, 'fixed': NUMBER_SCHEMA, 'integer': NUMBER_SCHEMA, 'numeric': NUMBER_SCHEMA, 'text': STRING_SCHEMA, 'string': STRING_SCHEMA, 'timestamp_ntz': DATETIME_SCHEMA, 'datetime': DATETIME_SCHEMA, 'date': DATETIME_SCHEMA, 'variant': STRING_SCHEMA, 'array': STRING_SCHEMA, 'object': STRING_SCHEMA, } def schema_for_column(col, inclusion='available'): """datatype checking for columns""" schema_kwargs = SCHEMA_MAPPING.get(col.data_type.lower()) if schema_kwargs: return Schema(**schema_kwargs, inclusion=inclusion) return Schema(None, inclusion='unsupported', description=f'Unsupported column type {col.data_type}')

Romil - this has been taken care of.

…th test_connection.py as per the feedback

vishalp-dev · 2025-06-18T12:41:41Z

+    """Maps Snowflake data types to Singer schema definitions."""
+    STRING_SCHEMA = {'type': ['null', 'string']}
+    NUMBER_SCHEMA = {'type': ['null', 'number']}
+    DATETIME_SCHEMA = {'type': ['null', 'string'], 'format': 'date-time'}
+
+    SCHEMA_MAPPING = {
+        'boolean': {'type': ['null', 'boolean']},
+        'number': NUMBER_SCHEMA,
+        'real': NUMBER_SCHEMA,
+        'float': NUMBER_SCHEMA,
+        'fixed': NUMBER_SCHEMA,
+        'integer': NUMBER_SCHEMA,
+        'numeric': NUMBER_SCHEMA,
+        'text': STRING_SCHEMA,
+        'string': STRING_SCHEMA,
+        'timestamp_ntz': DATETIME_SCHEMA,
+        'datetime': DATETIME_SCHEMA,
+        'date': DATETIME_SCHEMA,
+        'variant': STRING_SCHEMA,
+        'array': STRING_SCHEMA,
+        'object': STRING_SCHEMA,
+    }


These should ideally be outside the function definition to avoid multiple re-initialization of the SCHEMA_MAPPING and SCHEMA's.

Romil - this has been taken care of.

RushiT0122 · 2025-06-30T14:58:21Z

    with metrics.job_timer('sync_table') as timer:
        timer.tags['table'] = catalog_entry.table
-        sync_incremental.sync_table(con, catalog_entry, state, columns)
+        #sync_incremental.sync_table(con, catalog_entry, state, columns)


Remove this redundant comment.

This is Done

RushiT0122 · 2025-06-30T14:58:53Z

-
 from singer import get_logger, metadata
-from utils import get_test_connection, ensure_test_table
+from utils import get_test_connection, _ensure_test_table  # correct import


Suggested change

from utils import get_test_connection, _ensure_test_table # correct import

from utils import get_test_connection, _ensure_test_table

This is Done

RushiT0122

Left some minor comments. Also please update backlog ticket details of TODO unittests work in the PR description.

RushiT0122 · 2025-07-01T09:35:53Z

      install_requires=[
-          'snowflake-connector-python==1.6.3',
+          'snowflake-connector-python==3.15.0',
          # https://github.com/snowflakedb/snowflake-connector-python/pull/208


This comment was related to asn1crypto dependancy which we have removed in this PR so this is redundant now, please remove this. Not holding the PR approval for this.

Removed outdated comment related to asn1crypto.

RushiT0122 · 2025-07-01T09:38:17Z


-@nottest
-def ensure_test_table(con, table_spec):
+@nottest 


Since we renamed method, I think we don't need @nottest annotation.

Either revert renaming or remove annotation.

Removed @nottest annotation.

RushiT0122 · 2025-07-01T11:52:51Z

@romilqlik I am not holding the PR but since we are not fixing tests in this PR, we should revert all test related changes.

romilqlik · 2025-07-01T13:07:37Z

@romilqlik I am not holding the PR but since we are not fixing tests in this PR, we should revert all test related changes.

Hello Rushikesh, I have reverted all the test related.

…ty and orchestration support

…olve non-discoverable field error

SAC-27712: updated detup.py with latest dependencies

1b942b0

mittal-tushar changed the title ~~SAC-27712: updated detup.py with latest dependencies~~ SAC-27712: updated setup.py with latest dependencies Jun 11, 2025

gl-romil added 3 commits June 12, 2025 18:17

SAC-27713: Finalized discovery and Sync logic

313a670

SAC-27712: Added unit/integration tests and circle ci config

d0b3ad0

Fix: Removed JSON validator step in circleci due to missing schemas

3acbce1

romilqlik force-pushed the gl-master branch 5 times, most recently from 38d99dd to 01c5ee7 Compare June 16, 2025 06:43

Fix: Corrected Folder for the unit tests

d0dab6d

romilqlik force-pushed the gl-master branch from 01c5ee7 to d0dab6d Compare June 16, 2025 12:09

gl-romil added 8 commits June 16, 2025 18:29

Fixed parameters

8bd4c79

Created clean state pyc file

3a8ab2e

Fix: Import discover_catalog directly for test discovery and incrment…

569a83d

…al sync

Fix: Test Import discover_catalog directly for test discovery and inc…

affe180

…rmental sync

Fix: Ensure tap_amplitude as editable in circle

9e60946

Added correct files (init and tests)

09bf387

Fix: Added test_connection.py to handle the extarction failure

85a4045

Final commit after testing

eaee6fc

RushiT0122 requested changes Jun 17, 2025

View reviewed changes

vishalp-dev reviewed Jun 17, 2025

View reviewed changes

gl-romil added 4 commits June 18, 2025 12:28

Fix: updated schema logic as per comments

d41ea68

fix: clean setup.py - moved nose handling to CircleCI confi

be0fbd2

Fix: Removed old test files, revert incrmental.py, added unittests wi…

a3dea7e

…th test_connection.py as per the feedback

Fix: Removed the environment variables as not using the tap-tester

25fb435

vishalp-dev requested changes Jun 18, 2025

View reviewed changes

gl-romil added 2 commits June 18, 2025 18:19

Fix: revove the test_discovery.py

ce854b9

Fix: updated utils

db88c69

RushiT0122 reviewed Jun 30, 2025

View reviewed changes

RushiT0122 requested changes Jun 30, 2025

View reviewed changes

gl-romil added 3 commits July 1, 2025 10:27

Fix: Minor comments

771b977

Added docstrings in utils.py

3664bc0

Corrected todo with proper JIRA ticket

a2ad000

RushiT0122 reviewed Jul 1, 2025

View reviewed changes

gl-romil added 2 commits July 1, 2025 15:29

remove annotation and redundant comment

a324c6b

Removed tests folder from gl-master

612dbdf

RushiT0122 approved these changes Jul 1, 2025

View reviewed changes

gl-romil added 2 commits July 1, 2025 17:31

Temporarily added tests folder for clean removal

2db81bc

Removed all nit test files form gl-master branch

23d8389

Pulled tests folder from master into gl-master

198ce09

vishalp-dev self-requested a review July 2, 2025 12:04

vishalp-dev approved these changes Jul 2, 2025

View reviewed changes

gl-romil and others added 7 commits July 10, 2025 20:31

Fix: Updated discovery and sync logic for replication key compatibili…

c2a2961

…ty and orchestration support

Fix: Updated discovery and sync logic for replication key compatibili…

91c2600

…ty and orchestration support

Fix: Selected and replication key

2614b1c

Fix: Removed invalid 'selected' metadata from discovery output to res…

0aa4bc7

…olve non-discoverable field error

Fix:Field level selection

6d10442

Fix automatic key property selection

a8cab43

Lint

8fd0d55

skuttleman mentioned this pull request Jul 14, 2025

SAC-27925: bump dependencies for compliance #11

Closed

1 task

Bump version 1.2.0

a1a23c4

dsprayberry merged commit 2c67a1c into master Jul 30, 2025

dsprayberry deleted the gl-master branch July 30, 2025 16:04

dsprayberry mentioned this pull request Jul 30, 2025

Bump snowflake-connector-python from 1.6.3 to 3.0.2 #6

Closed

	from utils import get_test_connection, _ensure_test_table # correct import
	from utils import get_test_connection, _ensure_test_table

Conversation

romilqlik commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Changes

Manual QA Steps

Risks

Rollback Steps

Uh oh!

singer-bot commented Jun 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RushiT0122 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romilqlik Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RushiT0122 commented Jul 1, 2025

Uh oh!

romilqlik commented Jul 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

romilqlik commented Jun 11, 2025 •

edited

Loading

romilqlik Jul 1, 2025 •

edited

Loading