Repository: astropy/astroquery

Issue:
TAP/TAP+ doesn't support the MAXREC parameter
Hello,

I am a maintainer of a TAP service, and some user has been complaining about the astroquery TAP implementation returning less rows than expected in some queries. Looking into it I saw that it lacks support for the [MAXREC parameter](http://www.ivoa.net/documents/TAP/20190927/REC-TAP-1.1.html#tth_sEc2.7.4), wherewith the user can specify the maximum number of records they want to fetch. In our case the default is 100.000 and for a query which should return more rows it would yield partial results.

For this very reason I had to write the python examples for our service with PyVO (another astropy affiliated package). But I heard that astroquery as a “catch-all library” might supersede it and I am looking into rewriting them.

I was looking into the docs and -- when I could not find it -- the source code and I saw that @jcsegovia from ESA is the person who developed this alongside the `gaia` submodule (#836). I have been able to implement it, but I would like to hear from him before creating a pull request. 

I feel like this is a key part of the functionality which has been overlooked...

Discussion:
bsipocz: I'm rebasing this now and fix the kwarg, so the change can end up in 0.4
bsipocz: Unfortunately is now unlikely to make the cut as I'm not certain that the modified tests are doing anything. 

E.g try to add one where maxrec is smaller than the otherwise expected result. E.g. as I see the expected result has 3 entry. While changing maxrec to 2 doesn't make those asserts fail.
bsipocz: This has now multiple conflicts. Would you @jontxu still interested in pursuing it? If yes, please rebase and address the API changing issue above. Making all optional kwargs kwarg only would be my preference, but that may cause issues with the modules using tapplus (so we have to be careful to check CI and remote_tests before merging if that change happens).
jontxu: Hi, sorry for the delay, I'm somewhat busy lately. but I'll look into it.

Patch:
diff --git a/astroquery/utils/tap/core.py b/astroquery/utils/tap/core.py
index 857e928c..cf14219c 100755
--- a/astroquery/utils/tap/core.py
+++ b/astroquery/utils/tap/core.py
@@ -237,7 +237,7 @@ class Tap:
     def launch_job(self, query, name=None, output_file=None,
                    output_format="votable", verbose=False,
                    dump_to_file=False, upload_resource=None,
-                   upload_table_name=None):
+                   upload_table_name=None, maxrec=None):
         """Launches a synchronous job
 
         Parameters
@@ -260,6 +260,8 @@ class Tap:
         upload_table_name : str, optional, default None
             resource temporary table name associated to the uploaded resource.
             This argument is required if upload_resource is provided.
+        maxrec : int, optional, default None
+            maximum number of rows to return (TAP ``MAXREC`` parameter)
 
         Returns
         -------
@@ -274,19 +276,21 @@ class Tap:
             if upload_table_name is None:
                 raise ValueError("Table name is required when a resource " +
                                  "is uploaded")
-            response = self.__launchJobMultipart(query,
-                                                 upload_resource,
-                                                 upload_table_name,
-                                                 output_format,
-                                                 "sync",
-                                                 verbose,
-                                                 name)
+            response = self.__launchJobMultipart(query=query,
+                                                 uploadResource=upload_resource,
+                                                 uploadTableName=upload_table_name,
+                                                 outputFormat=output_format,
+                                                 context="sync",
+                                                 verbose=verbose,
+                                                 name=name,
+                                                 maxrec=maxrec)
         else:
-            response = self.__launchJob(query,
-                                        output_format,
-                                        "sync",
-                                        verbose,
-                                        name)
+            response = self.__launchJob(query=query,
+                                        outputFormat=output_format,
+                                        context="sync",
+                                        verbose=verbose,
+                                        name=name,
+                                        maxrec=maxrec)
         # handle redirection
         if response.status == 303:
             # redirection
@@ -353,7 +357,7 @@ class Tap:
                          output_format="votable", verbose=False,
                          dump_to_file=False, background=False,
                          upload_resource=None, upload_table_name=None,
-                         autorun=True):
+                         autorun=True, maxrec=None):
         """Launches an asynchronous job
 
         Parameters
@@ -382,6 +386,8 @@ class Tap:
         autorun : boolean, optional, default True
             if 'True', sets 'phase' parameter to 'RUN',
             so the framework can start the job.
+        maxrec : int, optional, default None
+            maximum number of rows to return (TAP ``MAXREC`` parameter)
 
         Returns
         -------
@@ -404,14 +410,16 @@ class Tap:
                                                  "async",
                                                  verbose,
                                                  name,
-                                                 autorun)
+                                                 autorun,
+                                                 maxrec)
         else:
             response = self.__launchJob(query,
                                         output_format,
                                         "async",
                                         verbose,
                                         name,
-                                        autorun)
+                                        autorun,
+                                        maxrec)
         isError = self.__connHandler.check_launch_response_status(response,
                                                                   verbose,
                                                                   303,
@@ -571,7 +579,7 @@ class Tap:
 
     def __launchJobMultipart(self, query, uploadResource, uploadTableName,
                              outputFormat, context, verbose, name=None,
-                             autorun=True):
+                             autorun=True, maxrec=None):
         uploadValue = f"{uploadTableName},param:{uploadTableName}"
         args = {
             "REQUEST": "doQuery",
@@ -580,6 +588,8 @@ class Tap:
             "tapclient": str(TAP_CLIENT_ID),
             "QUERY": str(query),
             "UPLOAD": "" + str(uploadValue)}
+        if maxrec is not None:
+            args['MAXREC'] = maxrec
         if autorun is True:
             args['PHASE'] = 'RUN'
         if name is not None:
@@ -610,13 +620,15 @@ class Tap:
         return response
 
     def __launchJob(self, query, outputFormat, context, verbose, name=None,
-                    autorun=True):
+                    autorun=True, maxrec=None):
         args = {
             "REQUEST": "doQuery",
             "LANG": "ADQL",
             "FORMAT": str(outputFormat),
             "tapclient": str(TAP_CLIENT_ID),
             "QUERY": str(query)}
+        if maxrec is not None:
+            args['MAXREC'] = maxrec
         if autorun is True:
             args['PHASE'] = 'RUN'
         if name is not None:


Test patch:
diff --git a/astroquery/utils/tap/tests/test_tap.py b/astroquery/utils/tap/tests/test_tap.py
index 574094d5..6d5c5458 100644
--- a/astroquery/utils/tap/tests/test_tap.py
+++ b/astroquery/utils/tap/tests/test_tap.py
@@ -172,9 +172,10 @@ def test_launch_sync_job():
     connHandler.set_response(jobRequest, responseLaunchJob)
 
     with pytest.raises(Exception):
-        tap.launch_job(query)
+        tap.launch_job(query, maxrec=10)
 
     responseLaunchJob.set_status_code(200)
+
     job = tap.launch_job(query)
 
     assert job is not None
@@ -449,13 +450,14 @@ def test_abort_job():
         "REQUEST": "doQuery",
         "LANG": "ADQL",
         "FORMAT": "votable",
+        "MAXREC": 10,
         "tapclient": str(TAP_CLIENT_ID),
         "QUERY": str(query)}
     sortedKey = taputils.taputil_create_sorted_dict_key(dictTmp)
     req = f"async?{sortedKey}"
     connHandler.set_response(req, responseLaunchJob)
 
-    job = tap.launch_job_async(query, autorun=False)
+    job = tap.launch_job_async(query, autorun=False, maxrec=10)
     assert job is not None
     assert job.get_phase() == 'PENDING'
     # abort job
@@ -482,6 +484,7 @@ def test_job_parameters():
         "REQUEST": "doQuery",
         "LANG": "ADQL",
         "FORMAT": "votable",
+        "MAXREC": 10,
         "tapclient": str(TAP_CLIENT_ID),
         "QUERY": str(query)}
     sortedKey = taputils.taputil_create_sorted_dict_key(dictTmp)
@@ -501,7 +504,7 @@ def test_job_parameters():
     connHandler.set_response(req, responseResultsJob)
 
     responseResultsJob.set_status_code(200)
-    job = tap.launch_job_async(query, autorun=False)
+    job = tap.launch_job_async(query, maxrec=10, autorun=False)
     assert job is not None
     assert job.get_phase() == 'PENDING'