From f67aad0508559ff8fc406488236356eb6f1204aa Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 20:21:45 +0000 Subject: [PATCH 1/4] Fix find_source_in_db crash on empty Sources table (#265) query_region can return a columnless DataFrame when the Sources table has zero rows, which previously raised an unguarded pandas KeyError when the ra/dec columns were indexed. Now that case is detected and treated as "no spatial matches" so the first ingest into a fresh database with search_db=True no longer crashes; a KeyError from genuinely wrong ra_col_name/dec_col_name still raises AstroDBError. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016UKKrx5abQaERu4vRaZJrP --- astrodb_utils/sources.py | 51 ++++++++++++++++++++++++++++++---------- tests/test_sources.py | 22 +++++++++++++++++ 2 files changed, 61 insertions(+), 12 deletions(-) diff --git a/astrodb_utils/sources.py b/astrodb_utils/sources.py index d6bbe3c9..e3508301 100644 --- a/astrodb_utils/sources.py +++ b/astrodb_utils/sources.py @@ -19,6 +19,33 @@ logger = logging.getLogger(__name__) +def _handle_empty_query_region_result(db, db_name_matches, source, ra_col_name, dec_col_name, error): + """ + Interpret a KeyError raised by Database.query_region. + + query_region can return a columnless DataFrame when the Sources table has no rows, + which raises a KeyError when the ra/dec columns are subsequently indexed. If the + Sources table is actually empty, treat this as "no spatial matches" rather than + raising; otherwise the KeyError likely indicates incorrect ra_col_name/dec_col_name + arguments, so surface that to the user. + """ + if db.query(db.Sources).count() == 0: + logger.debug( + f"{source}: query_region found no columns to search (Sources table is empty); " + "treating as no spatial matches." + ) + return db_name_matches[:0] + + msg = ( + f"Error querying database with coordinates: {error}\n" + f"Using ra_col_name: {ra_col_name}, dec_col_name: {dec_col_name}.\n" + "Try setting `ra_col_name` and `dec_col_name` arguments to the " + "column names used in the Sources table.\n" + ) + logger.error(msg) + raise AstroDBError(msg) + + def find_source_in_db( db, source: str, @@ -113,9 +140,14 @@ def find_source_in_db( f"{source}: Trying coord in database search around " f"{location.ra.degree}, {location.dec}" ) - db_name_matches = db.query_region( - location, radius=search_radius, ra_col=ra_col_name, dec_col=dec_col_name - ) + try: + db_name_matches = db.query_region( + location, radius=search_radius, ra_col=ra_col_name, dec_col=dec_col_name + ) + except KeyError as e: + db_name_matches = _handle_empty_query_region_result( + db, db_name_matches, source, ra_col_name, dec_col_name, e + ) # If still no matches, try to get the coords from SIMBAD using source name if len(db_name_matches) == 0 and use_simbad: @@ -129,17 +161,12 @@ def find_source_in_db( logger.debug(msg2) try: db_name_matches = db.query_region( - simbad_skycoord, radius=search_radius, ra_col=ra_col_name, dec_col=dec_col_name - ) + simbad_skycoord, radius=search_radius, ra_col=ra_col_name, dec_col=dec_col_name + ) except KeyError as e: - msg = ( - f"Error querying database with coordinates from SIMBAD: {e}\n" - f"Using skycoord: {simbad_skycoord} and ra_col_name: {ra_col_name}, dec_col_name: {dec_col_name}.\n" - "Try setting `ra_col_name` and `dec_col_name` arguments to the " - "column names used in the Sources table.\n" + db_name_matches = _handle_empty_query_region_result( + db, db_name_matches, source, ra_col_name, dec_col_name, e ) - logger.error(msg) - raise AstroDBError(msg) if len(db_name_matches) == 1: db_names = db_name_matches["source"].tolist() diff --git a/tests/test_sources.py b/tests/test_sources.py index 0cb5dedc..ea1886ea 100644 --- a/tests/test_sources.py +++ b/tests/test_sources.py @@ -1,7 +1,9 @@ import math +import os import astropy.units as u import pytest +from astrodbkit.astrodb import Database, create_database from astrodb_utils import ( AstroDBError, @@ -111,6 +113,26 @@ def test_find_source_in_db(db): ) # This is wrong and a result of fuzzy matching +def test_find_source_in_db_empty_sources_table(tmp_path): + # Regression test for #265: the spatial fallback search in find_source_in_db + # used to crash with a pandas KeyError when the Sources table had zero rows. + template_schema_path = os.path.join("tests", "astrodb-template-db") + connection_string = f"sqlite:///{tmp_path}/empty_sources.sqlite" + create_database(connection_string, felis_schema=os.path.join(template_schema_path, "schema.yaml")) + empty_db = Database(connection_string) + + assert empty_db.query(empty_db.Sources).count() == 0 + + search_result = find_source_in_db( + empty_db, + "Test Source", + ra=10.0, + dec=20.0, + use_simbad=False, + ) + assert search_result == [] + + def test_find_source_in_db_with_coords(db): ingest_source( db, From e313199355d51b253a1970e63aed00d8ff3f7cf9 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 20:46:11 +0000 Subject: [PATCH 2/4] Preserve original error behavior when Sources table is non-empty The first query_region call site (direct ra/dec) previously let a KeyError from bad ra_col_name/dec_col_name propagate unwrapped, and test_find_source_in_db_errors relies on that. The shared empty-table guard from the previous commit incorrectly wrapped it into AstroDBError in all cases. Only wrap into AstroDBError when the Sources table is actually empty; otherwise re-raise the original KeyError (first call site) or the original AstroDBError message (second, SIMBAD-resolved call site), matching pre-existing behavior. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016UKKrx5abQaERu4vRaZJrP --- astrodb_utils/sources.py | 56 ++++++++++++++++++---------------------- 1 file changed, 25 insertions(+), 31 deletions(-) diff --git a/astrodb_utils/sources.py b/astrodb_utils/sources.py index e3508301..a997f682 100644 --- a/astrodb_utils/sources.py +++ b/astrodb_utils/sources.py @@ -19,31 +19,9 @@ logger = logging.getLogger(__name__) -def _handle_empty_query_region_result(db, db_name_matches, source, ra_col_name, dec_col_name, error): - """ - Interpret a KeyError raised by Database.query_region. - - query_region can return a columnless DataFrame when the Sources table has no rows, - which raises a KeyError when the ra/dec columns are subsequently indexed. If the - Sources table is actually empty, treat this as "no spatial matches" rather than - raising; otherwise the KeyError likely indicates incorrect ra_col_name/dec_col_name - arguments, so surface that to the user. - """ - if db.query(db.Sources).count() == 0: - logger.debug( - f"{source}: query_region found no columns to search (Sources table is empty); " - "treating as no spatial matches." - ) - return db_name_matches[:0] - - msg = ( - f"Error querying database with coordinates: {error}\n" - f"Using ra_col_name: {ra_col_name}, dec_col_name: {dec_col_name}.\n" - "Try setting `ra_col_name` and `dec_col_name` arguments to the " - "column names used in the Sources table.\n" - ) - logger.error(msg) - raise AstroDBError(msg) +def _empty_sources_table(db): + """Return True if the Sources table has zero rows.""" + return db.query(db.Sources).count() == 0 def find_source_in_db( @@ -144,10 +122,14 @@ def find_source_in_db( db_name_matches = db.query_region( location, radius=search_radius, ra_col=ra_col_name, dec_col=dec_col_name ) - except KeyError as e: - db_name_matches = _handle_empty_query_region_result( - db, db_name_matches, source, ra_col_name, dec_col_name, e + except KeyError: + if not _empty_sources_table(db): + raise + logger.debug( + f"{source}: query_region found no columns to search (Sources table is empty); " + "treating as no spatial matches." ) + db_name_matches = db_name_matches[:0] # If still no matches, try to get the coords from SIMBAD using source name if len(db_name_matches) == 0 and use_simbad: @@ -164,9 +146,21 @@ def find_source_in_db( simbad_skycoord, radius=search_radius, ra_col=ra_col_name, dec_col=dec_col_name ) except KeyError as e: - db_name_matches = _handle_empty_query_region_result( - db, db_name_matches, source, ra_col_name, dec_col_name, e - ) + if _empty_sources_table(db): + logger.debug( + f"{source}: query_region found no columns to search (Sources table is empty); " + "treating as no spatial matches." + ) + db_name_matches = db_name_matches[:0] + else: + msg = ( + f"Error querying database with coordinates from SIMBAD: {e}\n" + f"Using skycoord: {simbad_skycoord} and ra_col_name: {ra_col_name}, dec_col_name: {dec_col_name}.\n" + "Try setting `ra_col_name` and `dec_col_name` arguments to the " + "column names used in the Sources table.\n" + ) + logger.error(msg) + raise AstroDBError(msg) if len(db_name_matches) == 1: db_names = db_name_matches["source"].tolist() From cc3d3ca8f8a4c55e780030c6c26d487813381598 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 20:48:34 +0000 Subject: [PATCH 3/4] Simplify: check for empty Sources table up front Replace the two try/except KeyError wrappers around query_region with a single early return when the Sources table has zero rows. An empty table can have no matches by definition, so this is simpler, avoids wasted SIMBAD/fuzzy search work, and keeps both query_region call sites' original error behavior completely unchanged for the non-empty-table case. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016UKKrx5abQaERu4vRaZJrP --- astrodb_utils/sources.py | 49 ++++++++++++++-------------------------- 1 file changed, 17 insertions(+), 32 deletions(-) diff --git a/astrodb_utils/sources.py b/astrodb_utils/sources.py index a997f682..e566c206 100644 --- a/astrodb_utils/sources.py +++ b/astrodb_utils/sources.py @@ -19,11 +19,6 @@ logger = logging.getLogger(__name__) -def _empty_sources_table(db): - """Return True if the Sources table has zero rows.""" - return db.query(db.Sources).count() == 0 - - def find_source_in_db( db, source: str, @@ -74,6 +69,12 @@ def find_source_in_db( source = source.strip() + # An empty Sources table has no matches by definition, and Database.query_region + # raises a KeyError (rather than returning zero rows) when queried against one. + if db.query(db.Sources).count() == 0: + logger.debug(f"{source}: Sources table is empty, no matches possible.") + return [] + logger.debug(f"{source}: Searching for match in database. Use Simbad: {use_simbad}") db_name_matches = db.search_object( source, @@ -118,18 +119,9 @@ def find_source_in_db( f"{source}: Trying coord in database search around " f"{location.ra.degree}, {location.dec}" ) - try: - db_name_matches = db.query_region( - location, radius=search_radius, ra_col=ra_col_name, dec_col=dec_col_name - ) - except KeyError: - if not _empty_sources_table(db): - raise - logger.debug( - f"{source}: query_region found no columns to search (Sources table is empty); " - "treating as no spatial matches." - ) - db_name_matches = db_name_matches[:0] + db_name_matches = db.query_region( + location, radius=search_radius, ra_col=ra_col_name, dec_col=dec_col_name + ) # If still no matches, try to get the coords from SIMBAD using source name if len(db_name_matches) == 0 and use_simbad: @@ -146,21 +138,14 @@ def find_source_in_db( simbad_skycoord, radius=search_radius, ra_col=ra_col_name, dec_col=dec_col_name ) except KeyError as e: - if _empty_sources_table(db): - logger.debug( - f"{source}: query_region found no columns to search (Sources table is empty); " - "treating as no spatial matches." - ) - db_name_matches = db_name_matches[:0] - else: - msg = ( - f"Error querying database with coordinates from SIMBAD: {e}\n" - f"Using skycoord: {simbad_skycoord} and ra_col_name: {ra_col_name}, dec_col_name: {dec_col_name}.\n" - "Try setting `ra_col_name` and `dec_col_name` arguments to the " - "column names used in the Sources table.\n" - ) - logger.error(msg) - raise AstroDBError(msg) + msg = ( + f"Error querying database with coordinates from SIMBAD: {e}\n" + f"Using skycoord: {simbad_skycoord} and ra_col_name: {ra_col_name}, dec_col_name: {dec_col_name}.\n" + "Try setting `ra_col_name` and `dec_col_name` arguments to the " + "column names used in the Sources table.\n" + ) + logger.error(msg) + raise AstroDBError(msg) if len(db_name_matches) == 1: db_names = db_name_matches["source"].tolist() From c5e664f69ee156c4c6818e3742b823c749ab4c73 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 20:55:59 +0000 Subject: [PATCH 4/4] Address review feedback: avoid full COUNT, dispose test engine - Use query(...).first() is None instead of count() == 0 to check for an empty Sources table, avoiding a full COUNT(*) scan. - Dispose the SQLAlchemy engine in test_find_source_in_db_empty_sources_table so the temporary sqlite file's handle is released. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016UKKrx5abQaERu4vRaZJrP --- astrodb_utils/sources.py | 2 +- tests/test_sources.py | 23 +++++++++++++---------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/astrodb_utils/sources.py b/astrodb_utils/sources.py index e566c206..30acc573 100644 --- a/astrodb_utils/sources.py +++ b/astrodb_utils/sources.py @@ -71,7 +71,7 @@ def find_source_in_db( # An empty Sources table has no matches by definition, and Database.query_region # raises a KeyError (rather than returning zero rows) when queried against one. - if db.query(db.Sources).count() == 0: + if db.query(db.Sources).first() is None: logger.debug(f"{source}: Sources table is empty, no matches possible.") return [] diff --git a/tests/test_sources.py b/tests/test_sources.py index ea1886ea..4c530087 100644 --- a/tests/test_sources.py +++ b/tests/test_sources.py @@ -121,16 +121,19 @@ def test_find_source_in_db_empty_sources_table(tmp_path): create_database(connection_string, felis_schema=os.path.join(template_schema_path, "schema.yaml")) empty_db = Database(connection_string) - assert empty_db.query(empty_db.Sources).count() == 0 - - search_result = find_source_in_db( - empty_db, - "Test Source", - ra=10.0, - dec=20.0, - use_simbad=False, - ) - assert search_result == [] + try: + assert empty_db.query(empty_db.Sources).count() == 0 + + search_result = find_source_in_db( + empty_db, + "Test Source", + ra=10.0, + dec=20.0, + use_simbad=False, + ) + assert search_result == [] + finally: + empty_db.engine.dispose() def test_find_source_in_db_with_coords(db):